adds secrets.#1
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a pluggable encryption interface and OpenSSL implementation, encrypted settings source and secret manager, layered SettingManager with factory, environment detection utility, Env source round-trip serialization, Memory constructor normalization, and extensive unit tests for these features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Factory as SettingManagerFactory
participant Detector as EnvironmentDetector
participant Manager as SettingManager
participant YAML as YAML Source
participant EncSrc as Encrypted Source
participant Encryptor as OpenSSLEncryptor
participant EnvSrc as Env Source
Client->>Factory: create(?environment, configPath)
Factory->>Detector: detect()
Detector-->>Factory: environment
rect rgb(236,244,255)
Note over Factory,Manager: Assemble prioritized sources (env vars highest)
Factory->>Manager: new()
Factory->>Manager: addSource(YAML base?)
Factory->>Manager: addSource(YAML env?)
Factory->>Manager: addSource(EncSrc credentials?)
Factory->>Manager: addSource(EnvSrc)
end
rect rgb(245,255,240)
Note over EncSrc,Encryptor: Load encrypted secrets if present
Factory->>EncSrc: new(credentialsPath, keyPath)
EncSrc->>Encryptor: decrypt(payload, key)
Encryptor-->>EncSrc: plaintext YAML
EncSrc-->>Manager: provide settings
end
Client->>Manager: get('section','key')
Manager->>Manager: iterate sources by priority
Manager-->>Client: value or null
sequenceDiagram
participant User
participant SecretMgr as SecretManager
participant FS as IFileSystem
participant Encryptor as OpenSSLEncryptor
participant Editor as TextEditor
User->>SecretMgr: edit(credentialsPath, keyPath, editor)
SecretMgr->>FS: file_exists(keyPath)?
alt key missing
SecretMgr->>Encryptor: generateKey()
Encryptor-->>SecretMgr: key
SecretMgr->>FS: write(keyPath) + chmod(0600)
end
SecretMgr->>FS: read(credentialsPath)?
alt credentials exist
SecretMgr->>Encryptor: decrypt(encryptedPayload, key)
Encryptor-->>SecretMgr: YAML content
else
SecretMgr-->>SecretMgr: create empty YAML
end
SecretMgr->>FS: write(tempFile, YAML) + chmod(0600)
SecretMgr->>Editor: launch(tempFile)
Editor-->>SecretMgr: edited file
SecretMgr->>SecretMgr: validate YAML
SecretMgr->>Encryptor: encrypt(YAML, key)
Encryptor-->>SecretMgr: encryptedPayload
SecretMgr->>FS: write(credentialsPath, encryptedPayload) (atomic rename)
SecretMgr->>FS: unlink(tempFile)
SecretMgr-->>User: success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @src/Data/Encryption/OpenSSLEncryptor.php:
- Around line 113-119: The decrypt() call is using $key directly (same hex-key
bug as encrypt()); before calling openssl_decrypt in OpenSSLEncryptor::decrypt
(and the $decrypted assignment), normalize $key the same way as in encrypt() by
converting a hex string to binary (e.g., if ctype_xdigit($key) or strlen
indicates hex, run $key = hex2bin($key)); then pass the binary $key into
openssl_decrypt so IV/key formats match encrypt().
- Around line 44-50: The encryption call in OpenSSLEncryptor::encrypt passes a
hex-encoded key directly to openssl_encrypt causing mismatched effective keys
with generateMac; convert the key to binary (pack or hex2bin) when it is a
64-char hex string before using it in openssl_encrypt (and similarly normalize
the key in OpenSSLEncryptor::decrypt after validation) so both encryption and
generateMac use the same binary key.
In @src/Data/Settings/SecretManager.php:
- Around line 60-62: The temporary filename creation in SecretManager uses
predictable uniqid() for $tempFile and writes decrypted secrets via
$this->fs->writeFile; replace the predictable suffix with a cryptographically
secure random token (e.g., from random_bytes or a secure helper) and incorporate
it into the filename, ensuring the generation and length are sufficient to
prevent guessing, then continue to write and securely clean up the file; update
the code paths that build $tempFile and any cleanup logic to use this new secure
token instead of uniqid().
In @src/Data/Settings/SettingManagerFactory.php:
- Around line 172-197: The private static method findKey() is dead code (never
called); remove the unused method from SettingManagerFactory or, if intended for
create(), integrate it by replacing the custom key resolution with calls to
SettingManagerFactory::findKey() inside create() and ensure compatibility with
the Encrypted class's getKey() behavior (i.e., use findKey() to locate
file/ENV/RAILS_MASTER_KEY and pass the resolved key into Encrypted where
create() constructs the Encrypted source).
🧹 Nitpick comments (8)
src/Data/Settings/EnvironmentDetector.php (1)
170-203: Consider caching the detected environment.Each call to
isProduction(),isDevelopment(),isTest(), orisStaging()invokesdetect(), which iterates through environment variables and performs string operations. If these methods are called frequently, consider caching the result:🔎 Optional caching approach
class EnvironmentDetector { + private static ?string $cachedEnvironment = null; + public static function detect(): string { + if( self::$cachedEnvironment !== null ) + { + return self::$cachedEnvironment; + } + // Check environment variables in priority order foreach( self::ENV_VARIABLES as $varName ) { // ... existing logic ... } - return 'development'; + self::$cachedEnvironment = 'development'; + return self::$cachedEnvironment; }src/Data/Settings/Source/Encrypted.php (2)
192-204: Silent exception swallowing insave()may hinder debugging.The catch block returns
falsewithout logging or exposing the exception. While silent failure may be intentional for optional encrypted sources, callers have no way to distinguish between "no key available" (line 187-190) and an actual encryption/write error.Consider at minimum logging the exception or providing a way to retrieve the last error:
🔎 Optional: Add error context
+ private ?\Exception $lastError = null; + + public function getLastError(): ?\Exception + { + return $this->lastError; + } + public function save(): bool { $key = $this->getKey(); if( !$key ) { return false; } try { $yaml = YamlParser::dump( $this->settings, 4, 2 ); $encrypted = $this->encryptor->encrypt( $yaml, $key ); $this->fs->writeFile( $this->credentialsPath, $encrypted ); return true; } catch( \Exception $e ) { + $this->lastError = $e; return false; } }
94-119: Duplicated key-finding logic withSettingManagerFactory::findKey().The
getKey()method duplicates the key resolution logic found inSettingManagerFactory::findKey()(lines 172-197). Both implement the same pattern: check file → check environment variable → check RAILS_MASTER_KEY.Consider extracting this to a shared utility (e.g.,
KeyResolveror a static method inSecretManager) to maintain DRY and ensure consistent behavior.src/Data/Settings/SettingManagerFactory.php (2)
38-41: Direct filesystem calls reduce testability.The factory uses direct
file_exists()calls (lines 38, 45, 60, 76, 113, 120) instead of theIFileSystemabstraction used elsewhere in this PR. This makes unit testing the factory more difficult.Consider accepting an optional
IFileSystemparameter or using a static file system accessor for consistency withSecretManagerandEncrypted.
62-71: Silent secret loading failures may hide configuration issues.When encrypted sources fail to load (lines 62-70, 78-86), exceptions are silently swallowed. While the comment explains this is intentional ("key might be in env var"), consider adding optional logging or a debug mode to help diagnose configuration problems in non-production environments.
src/Data/Settings/SecretManager.php (2)
185-186: Directchmod()bypassesIFileSystemabstraction.The
chmod()call on line 186 directly accesses the filesystem, bypassing the injected$this->fs. This breaks the abstraction and makes testing difficult. TheIFileSysteminterface should include achmod()method, or this should be documented as an intentional limitation.
222-235: Consider validating key paths differ inrotateKey().If
$oldKeyPathequals$newKeyPath, the method would overwrite the key file at line 228 after decryption succeeds. While currently safe (decryption happens first viashow()), explicit validation would prevent future refactoring issues:🔎 Add path validation
public function rotateKey( string $credentialsPath, string $oldKeyPath, string $newKeyPath ): bool { + if( realpath( $oldKeyPath ) === realpath( $newKeyPath ) ) + { + throw new \Exception( "Old and new key paths must differ for rotation." ); + } + // Decrypt with old key $content = $this->show( $credentialsPath, $oldKeyPath );src/Data/Settings/SettingManager.php (1)
156-184:set()immediately persists changes, which may be inefficient for batch updates.Each
set()call triggerssave()on the target source. For batch configuration updates, this could cause multiple disk writes or encryption operations. Consider deferring persistence or providing a batch API:🔎 Alternative: defer save to explicit call
- public function set( string $sectionName, string $name, mixed $value ): ISettingSource + public function set( string $sectionName, string $name, mixed $value, bool $persist = false ): ISettingSource { // Prefer additional sources (highest priority) if( !empty( $this->additionalSources ) ) { $lastSource = $this->additionalSources[count( $this->additionalSources ) - 1]['source']; $lastSource->set( $sectionName, $name, $value ); - $lastSource->save(); + if( $persist ) + { + $lastSource->save(); + } return $this; } // ... similar changes for other branches
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Data/Encryption/IEncryptor.phpsrc/Data/Encryption/OpenSSLEncryptor.phpsrc/Data/Settings/EnvironmentDetector.phpsrc/Data/Settings/SecretManager.phpsrc/Data/Settings/SettingManager.phpsrc/Data/Settings/SettingManagerFactory.phpsrc/Data/Settings/Source/Encrypted.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Data/Encryption/IEncryptor.php (2)
src/Data/Encryption/OpenSSLEncryptor.php (5)
encrypt(32-69)decrypt(79-127)generateKey(135-146)isValidKey(154-169)getCipher(176-179)src/Data/Settings/SecretManager.php (2)
encrypt(140-165)generateKey(175-189)
src/Data/Settings/SecretManager.php (3)
src/Data/Encryption/OpenSSLEncryptor.php (4)
OpenSSLEncryptor(13-216)decrypt(79-127)encrypt(32-69)generateKey(135-146)src/Data/Settings/Source/Encrypted.php (1)
__construct(35-48)src/Data/Encryption/IEncryptor.php (3)
decrypt(34-34)encrypt(24-24)generateKey(42-42)
src/Data/Settings/Source/Encrypted.php (3)
src/Data/Settings/Source/Yaml.php (1)
Yaml(14-123)src/Data/Settings/SecretManager.php (2)
__construct(29-33)encrypt(140-165)src/Data/Encryption/IEncryptor.php (2)
decrypt(34-34)encrypt(24-24)
src/Data/Settings/SettingManager.php (2)
src/Data/Settings/Source/ISettingSource.php (6)
get(10-10)set(11-11)save(15-15)getSectionNames(12-12)getSectionSettingNames(13-13)getSection(14-14)src/Data/Settings/Source/Memory.php (6)
get(18-31)set(40-44)save(81-84)getSectionNames(50-53)getSectionSettingNames(60-63)getSection(72-75)
🪛 PHPMD (2.15.0)
src/Data/Settings/SettingManagerFactory.php
172-197: Avoid unused private methods such as 'findKey'. (undefined)
(UnusedPrivateMethod)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
src/Data/Encryption/IEncryptor.php (1)
14-58: Well-designed encryption interface.The
IEncryptorinterface provides a clean contract for encryption backends with proper documentation, appropriate method signatures, and exception specifications. The design supports pluggable implementations (OpenSSL, Sodium, etc.) as mentioned in the docblock.src/Data/Encryption/OpenSSLEncryptor.php (2)
135-146: Key generation implementation is solid.Properly checks for cryptographic strength via the
$strongreference parameter and returns hex-encoded for safe storage.
209-215: Good use ofhash_equals()for timing-safe comparison.Using
hash_equals()prevents timing attacks when verifying the MAC.src/Data/Settings/SettingManager.php (3)
242-258: Shallow merge may unexpectedly replace nested configuration.
array_merge()performs a shallow merge. If sections contain nested arrays, higher-priority sources completely replace nested structures rather than merging them recursively. This could be surprising for deeply nested configurations.Consider documenting this behavior or using
array_replace_recursive()for deep merging if that aligns with intended semantics.
80-92: Clean source stacking with optional naming.The
addSource()implementation with optional$nameparameter for debugging purposes is a nice touch for troubleshooting configuration issues.
268-282: Aggregate save behavior is well-implemented.The
save()method correctly iterates all sources and returns overall success status while attempting to save all sources regardless of individual failures.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/Data/Settings/SecretManager.php:
- Around line 381-384: The code in SecretManager that checks environment
variables using isset($_ENV[$envKey]) can fail if php.ini's variables_order
omits "E"; replace that check with getenv($envKey) !== false and return the
value from getenv($envKey) (or null/empty if getenv returns false) so the code
reliably reads env vars across PHP configurations—locate the conditional in the
SecretManager class where $_ENV[$envKey] is referenced and update it to use
getenv($envKey) for both checking and returning the value.
♻️ Duplicate comments (1)
src/Data/Settings/SecretManager.php (1)
60-62: Predictable temp file naming and insecure permissions for sensitive data.This issue was previously flagged. The
uniqid()produces predictable filenames and the temp file is written without restrictive permissions, potentially exposing decrypted secrets.
🧹 Nitpick comments (3)
src/Data/Settings/SettingManagerFactory.php (1)
62-70: Silent exception swallowing may hide configuration errors.Catching all exceptions silently when loading encrypted sources makes debugging difficult. A corrupted file, invalid key format, or decryption error will fail silently with no indication to the user.
Consider logging the exception or providing a way to opt into strict mode:
🔎 Suggested approach
if( file_exists( $secretsPath ) ) { try { $encrypted = new Encrypted( $secretsPath, $masterKeyPath ); $manager->addSource( $encrypted, 'secrets' ); } catch( \Exception $e ) { - // Silently skip if secrets can't be loaded (key might be in env var) + // Log warning - secrets exist but couldn't be loaded + // error_log( "Warning: Could not load secrets from $secretsPath: " . $e->getMessage() ); } }Also applies to: 78-86
src/Data/Settings/SecretManager.php (2)
185-186: Directchmodbypasses the file system abstraction.Using
chmod()directly while the rest of the method uses$this->fsbreaks the abstraction and makes this code path untestable with a mock file system.🔎 Consider adding chmod support to IFileSystem
- // Ensure restrictive permissions (owner read/write only) - chmod( $keyPath, 0600 ); + // Ensure restrictive permissions (owner read/write only) + $this->fs->chmod( $keyPath, 0600 );This would require adding a
chmodmethod toIFileSystem.
224-363: Improved atomic key rotation with proper rollback.The refactored implementation addresses the previous atomicity concern with a well-structured approach: temp files → verification → backup → atomic rename → cleanup. The rollback logic in the exception handler properly restores original state on failure.
However, the method mixes
$this->fsabstraction with direct PHP calls (copy,rename,chmod), which breaks testability. Consider adding these operations toIFileSystemfor consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Data/Encryption/OpenSSLEncryptor.phpsrc/Data/Settings/EnvironmentDetector.phpsrc/Data/Settings/SecretManager.phpsrc/Data/Settings/SettingManagerFactory.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Data/Encryption/OpenSSLEncryptor.php
- src/Data/Settings/EnvironmentDetector.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
src/Data/Settings/SettingManagerFactory.php (3)
101-145: LGTM!The
createCustommethod provides flexible source configuration with appropriate null-safety checks before creating sources. The switch-case pattern handles the three supported source types cleanly.
153-159: LGTM!Clean helper for testing with in-memory configuration.
167-179: LGTM!Useful helper for documenting and validating the expected configuration structure.
src/Data/Settings/SecretManager.php (4)
29-33: LGTM!Good dependency injection with sensible defaults for production use while enabling testability.
113-129: LGTM!The method properly validates both files exist before attempting decryption.
140-165: LGTM!Good defensive validation of YAML syntax before encrypting, preventing storage of malformed configuration.
198-209: LGTM!Simple validation pattern that correctly uses
show()to verify decryptability.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/Data/Settings/SettingManagerFactory.php:
- Around line 36-55: In SettingManagerFactory, remove the unreachable else
branch that calls addSource for the environment YAML: since a fresh
SettingManager (created in the factory) will always have getSource() === null
after optional setFallback(), simply call setSource(new Yaml($envConfigPath))
for the environment config and delete the else block that attempts addSource; if
you need a flexible behavior for already-initialized managers, implement that in
a separate method rather than leaving dead code in the factory.
♻️ Duplicate comments (1)
src/Data/Settings/SecretManager.php (1)
60-62: Temp file containing decrypted secrets should have restrictive permissions.The temporary file written at line 62 contains decrypted credentials but is created with default permissions (typically 0644). On multi-user systems, other users could read the secrets during the edit window. Apply
chmod($tempFile, 0600)immediately after creation, similar to how key files are protected at line 186.🔎 Proposed fix
// Create temporary file with cryptographically secure token $tempFile = sys_get_temp_dir() . '/neuron_credentials_' . $this->generateSecureToken() . '.yml'; $this->fs->writeFile( $tempFile, $content ); + chmod( $tempFile, 0600 );
🧹 Nitpick comments (4)
src/Data/Settings/Source/Env.php (1)
105-131: Serialization logic is well-implemented with one edge case consideration.The serialization handles arrays, objects, booleans, null, and scalars appropriately. However, note that
nullserializes to''(empty string), andparseValue()at line 60 returns empty string as-is rather than converting back tonull. This meansset('s', 'n', null)followed byget('s', 'n')returns''instead ofnull.If round-trip fidelity for
nullis important, consider using a sentinel value or adjustingparseValue()to returnnullfor empty strings.src/Data/Settings/EnvironmentDetector.php (1)
176-209: Consider caching the detected environment for repeated calls.Each helper method (
isProduction(),isDevelopment(), etc.) callsdetect()independently, which re-runs all environment checks. For applications making many environment checks, consider caching the result in a static variable.src/Data/Settings/Source/Encrypted.php (1)
197-208: Silent exception swallowing may hinder debugging.The
save()method catches all exceptions and returnsfalse, losing error context. Consider logging the exception message or adding a separate method that throws on failure for use cases where error details matter.src/Data/Settings/SettingManagerFactory.php (1)
101-145: Consider documenting the priority order for custom sources.Sources added first via
addSource()will have lower priority than sources added later. This may not be obvious to callers. Consider documenting that the array order determines priority (first = lowest, last = highest), or providing a way to specify priority explicitly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
VERSIONLOG.mdsrc/Data/Settings/EnvironmentDetector.phpsrc/Data/Settings/SecretManager.phpsrc/Data/Settings/SettingManagerFactory.phpsrc/Data/Settings/Source/Encrypted.phpsrc/Data/Settings/Source/Env.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Data/Settings/Source/Env.php (1)
src/Data/Env.php (1)
put(138-141)
src/Data/Settings/SecretManager.php (3)
src/Data/Encryption/OpenSSLEncryptor.php (4)
OpenSSLEncryptor(13-228)decrypt(85-139)encrypt(32-75)generateKey(147-158)src/Data/Settings/Source/Yaml.php (1)
Yaml(14-123)src/Data/Encryption/IEncryptor.php (3)
decrypt(34-34)encrypt(24-24)generateKey(42-42)
src/Data/Settings/Source/Encrypted.php (4)
src/Data/Encryption/OpenSSLEncryptor.php (3)
OpenSSLEncryptor(13-228)decrypt(85-139)encrypt(32-75)src/Data/Settings/SecretManager.php (2)
__construct(29-33)encrypt(140-165)src/Data/Settings/Source/Env.php (3)
__construct(12-15)get(32-44)set(100-136)src/Data/Encryption/IEncryptor.php (2)
decrypt(34-34)encrypt(24-24)
src/Data/Settings/SettingManagerFactory.php (5)
src/Data/Settings/Source/Yaml.php (1)
Yaml(14-123)src/Data/Settings/Source/Encrypted.php (1)
Encrypted(20-210)src/Data/Settings/SettingManager.php (5)
SettingManager(15-283)setFallback(74-78)getSource(41-44)setSource(52-56)addSource(88-92)src/Data/Settings/EnvironmentDetector.php (2)
EnvironmentDetector(28-231)detect(55-91)src/Data/Env.php (1)
getInstance(92-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
VERSIONLOG.md (1)
1-2: LGTM!The version log entry accurately summarizes the new encrypted secrets functionality.
src/Data/Settings/EnvironmentDetector.php (1)
55-91: LGTM!The environment detection logic is well-structured with clear priority ordering. The documentation appropriately warns that production must explicitly set
APP_ENV. The previous CLI detection concern has been addressed.src/Data/Settings/Source/Encrypted.php (1)
55-87: LGTM with a note on error handling.The
loadSettings()method appropriately handles the case where files or keys are missing by returning empty settings, which is documented behavior for optional environment-specific secrets. The exception wrapping when decryption fails provides useful context.src/Data/Settings/SecretManager.php (2)
224-363: Well-implemented atomic key rotation with proper safeguards.The
rotateKey()method now follows an atomic pattern: decrypt → generate new key to temp → re-encrypt to temp → verify → backup originals → move temps → cleanup. The verification step ensures data integrity, and the catch block attempts to restore from backups on failure. This addresses the previous review concern about unrecoverable data loss.
404-419: LGTM!The secure token generation using
random_bytes()properly addresses the previous review concern about predictableuniqid()filenames.src/Data/Settings/SettingManagerFactory.php (1)
89-90: Clarification: The singleton pattern is correctly used here.A past review flagged this as an error, but the code is correct. There are two distinct
Envclasses:
\Neuron\Data\Env(singleton for raw env var access, obtained viagetInstance())\Neuron\Data\Settings\Source\Env(settings source that wraps the singleton)Line 90 correctly constructs a
Source\Envby passing the\Neuron\Data\Envsingleton obtained viagetInstance().
There was a problem hiding this comment.
Actionable comments posted: 7
Fix all issues with AI Agents 🤖
In @src/Data/Settings/SecretManager.php:
- Around line 255-256: The variable $credentialsUpdatedWithNewKey is assigned
but never used; either remove its declaration/assignment (delete
$credentialsUpdatedWithNewKey and any related comment) or use it in the rollback
check for clarity by replacing the current condition
($this->fs->readFile($credentialsPath) !== $encrypted) with a test that consults
$credentialsUpdatedWithNewKey (e.g. only perform rollback if
$credentialsUpdatedWithNewKey is true), and ensure the variable is properly set
where credentials are updated so the rollback logic reflects the intended state.
In @tests/Data/Encryption/OpenSSLEncryptorTest.php:
- Around line 211-226: testIVExtractionFromCiphertext incorrectly treats the
output of OpenSSLEncryptor::encrypt as a raw base64 blob; instead parse the JSON
payload returned by encrypt, decode the 'value' and 'iv' fields (base64_decode
on both if iv is base64-encoded, or use raw iv string per implementation),
assert the decoded value length is at least MAC (32) + ciphertext and that the
iv has expected 16 bytes, and update the comment to describe the JSON format
(e.g. {iv, value, mac}). Locate this logic in the testIVExtractionFromCiphertext
method and replace the base64_decode($encrypted) usage and wrong comment with
JSON decoding and proper assertions against 'iv' and decoded 'value'.
- Around line 54-68: The test defines an unused variable $encrypted2 (result of
$this->encryptor->encrypt($plaintext, $hexKeyLower)) which should be removed or
used; fix by either deleting the $encrypted2 assignment or replace the test to
assert that decrypting $encrypted2 with $hexKeyMixed equals $plaintext (use
$this->encryptor->decrypt($encrypted2, $hexKeyMixed) and assertEquals), ensuring
the test still verifies case-insensitive hex key handling in methods encrypt and
decrypt.
In @tests/Data/Settings/EnvironmentDetectorTest.php:
- Around line 15-27: The setUp method fails to save and clear NEURON_ENV, which
EnvironmentDetector checks; update setUp to store getenv('NEURON_ENV') into
$this->originalEnv['NEURON_ENV'] and call putenv('NEURON_ENV') to clear it, so
methods like setUp and tearDown around $this->originalEnv and putenv handle
NEURON_ENV the same way as APP_ENV, ENVIRONMENT, and APPLICATION_ENV.
In @tests/Data/Settings/SecretManagerTest.php:
- Around line 244-246: The test declares an unused variable $oldKey in
SecretManagerTest (the line "$oldKey = bin2hex( random_bytes( 32 ) );"); remove
that declaration so the test no longer defines an unused variable, leaving the
rest of the test logic unchanged.
In @tests/Data/Settings/Source/EnvRoundTripTest.php:
- Around line 21-32: The tearDown() method is missing cleanup for environment
variables created by tests; update the tearDown() in the EnvRoundTripTest class
to also unset TEST_MANUAL, TEST_EMPTY, and TEST_CSV (use putenv('TEST_MANUAL'),
putenv('TEST_EMPTY'), putenv('TEST_CSV')) alongside the existing putenv calls so
all test-set environment variables are cleared after each test.
♻️ Duplicate comments (1)
src/Data/Settings/SettingManagerFactory.php (1)
146-152: VerifyMemoryclass accepts constructor configuration.A past review comment indicated that the
Memoryclass may not accept constructor parameters, which would mean$configis silently ignored. Verify thatMemorycan be initialized with the provided configuration.#!/bin/bash # Check Memory class constructor signature ast-grep --pattern $'class Memory { $$$ __construct($$$) { $$$ } $$$ }' # Also search for Memory class definition rg -n "class Memory" --type=php -A 10
🧹 Nitpick comments (8)
tests/Data/Encryption/OpenSSLEncryptorTest.php (1)
8-16: Consider adding tests for decrypt validation paths.The test suite provides good coverage but could be enhanced with tests for additional error paths in
OpenSSLEncryptor::decrypt():
- Invalid JSON format (malformed JSON string)
- Missing required fields in payload (e.g., missing 'iv' or 'mac')
- Cipher mismatch (payload with different cipher algorithm)
- Direct test for
isValidKey()with various invalid formatstests/Data/Settings/Source/EncryptedTest.php (1)
314-348: Test may not align with actual key resolution logic.Looking at the
Encryptedsource class'sgetKey()method (lines 89-111 in the relevant snippet), it derives the environment variable name asNEURON_<BASENAME>_KEYfrom the key path. For/some/path/master.key, this would beNEURON_MASTER_KEY. Additionally, there's special handling forRAILS_MASTER_KEYwhen the key file ismaster.key.The test sets
NEURON_MASTER_KEYand uses key path/some/path/master.key, so the derived env var name would beNEURON_MASTER_KEY(frombasename('/some/path/master.key', '.key')= 'master'). This appears correct.However, the test should also verify the
RAILS_MASTER_KEYfallback path for completeness.Consider adding a separate test for
RAILS_MASTER_KEYcompatibility:public function testRailsMasterKeyEnvironmentVariable(): void { $railsKey = 'rails_master_key_12345678901234567890123456789'; $encrypted = 'encrypted'; $decrypted = "secret:\n value: from_rails_key"; putenv( 'RAILS_MASTER_KEY=' . $railsKey ); // ... similar mock setup with master.key path }tests/Data/Settings/SettingManagerFactoryTest.php (1)
179-195: Test does not verify that configuration values are accessible.The test only checks that
createForTestingreturns aSettingManagerinstance but doesn't verify that the provided$configvalues are actually accessible through the manager. Given the past review comment aboutMemoryclass potentially not accepting constructor parameters, this test should verify values can be retrieved.🔎 Proposed enhancement
public function testCreateForTesting(): void { $config = [ 'test' => [ 'key1' => 'value1', 'key2' => 'value2' ], 'another' => [ 'setting' => 'value' ] ]; $manager = SettingManagerFactory::createForTesting( $config ); - // Memory source needs flattened keys $this->assertInstanceOf( SettingManager::class, $manager ); + + // Verify configuration values are accessible + $this->assertEquals( 'value1', $manager->get( 'test', 'key1' ) ); + $this->assertEquals( 'value2', $manager->get( 'test', 'key2' ) ); + $this->assertEquals( 'value', $manager->get( 'another', 'setting' ) ); }src/Data/Settings/SettingManagerFactory.php (1)
55-64: Silent exception handling may hide configuration issues.While silently skipping encrypted sources when keys are unavailable is reasonable for optional environment-specific secrets, catching all exceptions without logging makes debugging difficult. Consider at least logging a debug message when secrets fail to load.
🔎 Suggested improvement
catch( \Exception $e ) { - // Silently skip if secrets can't be loaded (key might be in env var) + // Silently skip if secrets can't be loaded + // Consider logging for debugging: error_log("Failed to load secrets: " . $e->getMessage()); }Also applies to: 71-80
tests/Data/Settings/EnvironmentDetectorTest.php (1)
80-91: Consider includingNEURON_ENVin priority order test.For complete coverage of the priority chain, consider adding
NEURON_ENVto verify it takes precedence overAPPLICATION_ENVandENVIRONMENT.🔎 Suggested improvement
public function testPriorityOrder(): void { putenv( 'APP_ENV=production' ); + putenv( 'NEURON_ENV=test' ); putenv( 'ENVIRONMENT=staging' ); putenv( 'APPLICATION_ENV=testing' ); $env = EnvironmentDetector::detect(); $this->assertEquals( 'production', $env ); + + // Also verify NEURON_ENV takes precedence when APP_ENV is unset + putenv( 'APP_ENV' ); + $env = EnvironmentDetector::detect(); + $this->assertEquals( 'test', $env ); }tests/Data/Settings/SecretManagerTest.php (3)
84-84: Avoid using error suppression operator@.Using
@hides potential warnings or errors that could indicate problems. IfgenerateKey()produces expected warnings (e.g., fromchmodon a mock path), consider refactoring to avoid them or explicitly test for them.🔎 Suggested fix
- $key = @$this->secretManager->generateKey( $this->testKeyPath ); + $key = $this->secretManager->generateKey( $this->testKeyPath );If
chmodwarnings are expected on mock paths, consider mocking it or usingset_error_handler()to capture specific warnings.
163-175:withConsecutive()is deprecated in PHPUnit 10.While this works in PHPUnit 9.0.0,
withConsecutive()is deprecated and removed in PHPUnit 10+. Consider usingwillReturnCallback()for future compatibility. Also, the@error suppression on line 171 should be avoided.🔎 Suggested refactor for PHPUnit 10 compatibility
- $this->mockFileSystem->expects( $this->exactly( 2 ) ) - ->method( 'writeFile' ) - ->withConsecutive( - [$this->testKeyPath, $key], - [$this->testCredentialsPath, $encryptedContent] - ) - ->willReturn( 100, 200 ); - - $result = @$this->secretManager->encrypt( + $writeFileCalls = []; + $this->mockFileSystem->expects( $this->exactly( 2 ) ) + ->method( 'writeFile' ) + ->willReturnCallback( function( $path, $content ) use ( &$writeFileCalls, $key, $encryptedContent ) { + $writeFileCalls[] = [$path, $content]; + return 100; + } ); + + $result = $this->secretManager->encrypt(
286-331: Test cleanup is not robust if test fails early.The
$newKeyPathfile cleanup on line 330 only executes if the test passes. If an assertion fails before that point, the file will remain. Consider adding$newKeyPathto the class-level cleanup or usingtry/finally.Also,
withConsecutive()(lines 295-299) is deprecated in PHPUnit 10.🔎 Suggested fix for robust cleanup
public function testRotateKeySuccessfullyRotatesKeys(): void { // Use real filesystem for this integration test $realFs = new \Neuron\Core\System\RealFileSystem(); $realEncryptor = $this->createMock( IEncryptor::class ); $realSecretManager = new SecretManager( $realEncryptor, $realFs ); $oldKey = bin2hex( random_bytes( 32 ) ); $newKey = bin2hex( random_bytes( 32 ) ); $content = "database:\n password: secret123"; $oldEncrypted = base64_encode( 'old_encrypted_' . $content ); $newEncrypted = base64_encode( 'new_encrypted_' . $content ); $newKeyPath = '/tmp/test_new.key'; - // Setup initial files - file_put_contents( $this->testKeyPath, $oldKey ); - file_put_contents( $this->testCredentialsPath, $oldEncrypted ); + try + { + // Setup initial files + file_put_contents( $this->testKeyPath, $oldKey ); + file_put_contents( $this->testCredentialsPath, $oldEncrypted ); // ... rest of the test ... - // Clean up - unlink( $newKeyPath ); + } + finally + { + // Clean up + if( file_exists( $newKeyPath ) ) + { + unlink( $newKeyPath ); + } + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Data/Settings/SecretManager.phpsrc/Data/Settings/SettingManagerFactory.phpsrc/Data/Settings/Source/Env.phptests/Data/Encryption/OpenSSLEncryptorTest.phptests/Data/Setting/Source/EnvTest.phptests/Data/Settings/EnvironmentDetectorTest.phptests/Data/Settings/SecretManagerTest.phptests/Data/Settings/SettingManagerFactoryTest.phptests/Data/Settings/Source/EncryptedTest.phptests/Data/Settings/Source/EnvRoundTripTest.php
🧰 Additional context used
🧬 Code graph analysis (7)
tests/Data/Settings/Source/EncryptedTest.php (1)
src/Data/Settings/Source/Encrypted.php (1)
Encrypted(20-210)
tests/Data/Settings/SecretManagerTest.php (1)
src/Data/Settings/SecretManager.php (6)
SecretManager(20-456)generateKey(178-192)show(116-132)encrypt(143-168)validate(201-212)rotateKey(227-399)
src/Data/Settings/SecretManager.php (2)
src/Data/Encryption/OpenSSLEncryptor.php (4)
OpenSSLEncryptor(13-228)decrypt(85-139)encrypt(32-75)generateKey(147-158)src/Data/Encryption/IEncryptor.php (3)
decrypt(34-34)encrypt(24-24)generateKey(42-42)
tests/Data/Settings/Source/EnvRoundTripTest.php (1)
src/Data/Settings/Source/Env.php (3)
Env(8-267)set(110-146)get(32-44)
tests/Data/Settings/EnvironmentDetectorTest.php (1)
src/Data/Settings/EnvironmentDetector.php (2)
EnvironmentDetector(28-231)detect(55-91)
tests/Data/Encryption/OpenSSLEncryptorTest.php (1)
src/Data/Encryption/OpenSSLEncryptor.php (1)
OpenSSLEncryptor(13-228)
src/Data/Settings/Source/Env.php (1)
src/Data/Env.php (1)
put(138-141)
🪛 Gitleaks (8.30.0)
tests/Data/Settings/Source/EncryptedTest.php
[high] 43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 83-83: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/Data/Encryption/OpenSSLEncryptorTest.php
[high] 23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 58-58: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 PHPMD (2.15.0)
tests/Data/Settings/SecretManagerTest.php
139-139: Avoid unused local variables such as '$testKeyPath'. (undefined)
(UnusedLocalVariable)
245-245: Avoid unused local variables such as '$oldKey'. (undefined)
(UnusedLocalVariable)
src/Data/Settings/SecretManager.php
255-255: Avoid unused local variables such as '$credentialsUpdatedWithNewKey'. (undefined)
(UnusedLocalVariable)
tests/Data/Encryption/OpenSSLEncryptorTest.php
63-63: Avoid unused local variables such as '$encrypted2'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (25)
tests/Data/Encryption/OpenSSLEncryptorTest.php (1)
23-23: Static analysis: Gitleaks false positives for test fixtures.Gitleaks flagged the hardcoded hex strings as potential secrets. These are test fixtures, not real secrets, so the warnings are false positives. However, they may cause issues in automated security scanning pipelines.
If your CI/CD includes secret scanning that treats these as failures, consider:
- Adding inline Gitleaks ignore comments
- Generating test keys dynamically via
generateKey()instead of hardcodingFor reference, the inline ignore format is:
$hexKey = 'a1b2c3d4e5f67890...'; // gitleaks:allowAlso applies to: 56-58
tests/Data/Settings/Source/EncryptedTest.php (3)
43-43: Static analysis false positive – test fixture keys are safe.The Gitleaks warnings about "Generic API Key" on lines 43 and 83 are false positives. These are clearly test fixture keys with obvious prefixes like
test_encryption_key_andkey_from_environment_variable_. Test files commonly contain such placeholder secrets for mocking purposes.Also applies to: 83-83
17-36: Environment cleanup is correct.The
setUp()andtearDown()methods properly clear test environment variables usingputenv('VAR')(without=to unset). This ensures test isolation.
1-374: Comprehensive test coverage for Encrypted source.The test file provides good coverage of key resolution scenarios including file-based keys, environment variable fallback, missing files/keys, and value retrieval. The helper method
setupMocksForSuccessfulDecryptionreduces duplication effectively.tests/Data/Setting/Source/EnvTest.php (1)
245-258: Change correctly addresses null round-tripping.The test rename and assertion change from empty string to
nullaligns with theEnv.phpchanges whereset(null)serializes to empty string andget()now deserializes empty string back tonull. This ensures proper round-trip behavior.src/Data/Settings/Source/Env.php (3)
57-71: Boolean and null deserialization now implemented correctly.The
parseValue()method now properly handles:
- Empty string →
null(for null round-tripping)'true'→true(boolean)'false'→false(boolean)This addresses the previous review comment about boolean/null values not round-tripping correctly.
115-141: Proper serialization with JSON error handling.The
set()method now correctly serializes non-scalar values to JSON with appropriate error handling. TheRuntimeExceptionwith a descriptive message helps debugging serialization failures.
84-89: Comma-separated parsing is documented and intentional behaviorThe automatic comma-separated value parsing is explicitly documented in the
get()method docblock as part of the supported automatic array parsing feature. Multiple comprehensive tests confirm this is intentional design (testCommaSeparatedArrayParsing, testCommaSeparatedWithSpaces, testSingleValueCommaSeparated), with'value1,value2,value3'expected to parse as['value1', 'value2', 'value3']. The parsing hierarchy prioritizes JSON detection first, so JSON arrays are unambiguous, and comma-separated parsing only applies to plain string values. No changes needed.Likely an incorrect or invalid review comment.
tests/Data/Settings/SettingManagerFactoryTest.php (2)
73-95: Good use of try-finally for cleanup.The test properly uses
try-finallyblocks to ensure temporary files and directories are cleaned up regardless of test outcome.
12-30: Proper test isolation with singleton reset and env cleanup.The
setUpandtearDownmethods correctly reset theEnvsingleton and clean up test environment variables to ensure test isolation.src/Data/Settings/SettingManagerFactory.php (1)
31-86: Layered configuration correctly implements priority ordering.The factory correctly implements the configuration hierarchy with environment variables at highest priority. The conditional checks for file existence before attempting to load sources prevents unnecessary errors.
src/Data/Settings/SecretManager.php (3)
60-65: Secure temp file handling implemented correctly.The temp file now uses
generateSecureToken()for unpredictable naming and setschmod($tempFile, 0600)immediately after creation. This addresses previous review concerns about predictable filenames and world-readable permissions.
227-399: Comprehensive atomic key rotation with rollback.The
rotateKey()method implements a thorough atomic operation with:
- Backup creation before modifications
- Verification of re-encryption before committing
- Emergency key preservation when rollback fails
- Detailed error messages for manual intervention
The complexity is justified for preventing data loss during key rotation failures.
417-421: Environment variable key fallback usesgetenv()correctly.The
ensureKey()method correctly usesgetenv()instead of$_ENV, addressing the previous review concern about PHP configuration dependencies onvariables_order.tests/Data/Settings/Source/EnvRoundTripTest.php (2)
177-186: Important edge case: boolean strings preserved in arrays.This test correctly verifies that
'true'and'false'strings within JSON arrays are preserved as strings rather than converted to booleans. This is critical for proper round-tripping of array data.
37-67: Boolean and null round-trip tests validate the Env.php changes.These tests confirm that the serialization/deserialization logic in
Env.phpnow correctly handles boolean and null values. This directly validates the fix for the previously reported round-trip issue.tests/Data/Settings/EnvironmentDetectorTest.php (5)
46-66: LGTM!Both tests correctly verify detection via
APP_ENVandENVIRONMENTvariables respectively.
93-115: LGTM!Good test coverage for default behavior and CLI context handling. The CLI test documents an important design decision.
117-139: LGTM!Good edge case coverage for empty and whitespace-only environment variables.
141-168: LGTM!Excellent data-driven test covering normalization of standard names, aliases, and invalid environment names.
170-211: LGTM!Good coverage for custom/invalid environment names and verification that
getenv()is used for cross-configuration compatibility.tests/Data/Settings/SecretManagerTest.php (4)
44-64: LGTM!The cleanup method correctly handles glob patterns for test file cleanup.
102-130: LGTM!The test correctly verifies the show/decrypt flow with proper mock expectations including the key trimming behavior.
180-235: LGTM!Good test coverage for both successful validation and failure scenarios when decryption throws an exception.
333-371: LGTM!Thorough testing of secure token generation including length validation, hex format verification, and uniqueness across multiple samples. Using reflection to test a private security-critical method is appropriate here.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/Data/Settings/SecretManager.php:
- Around line 234-237: The in-place rotation check uses realpath($oldKeyPath)
=== realpath($newKeyPath) which returns false for non-existent paths and will
misdetect rotations; instead ensure you only call/compare realpath results when
both paths exist (e.g., file_exists on $oldKeyPath and $newKeyPath) or normalize
and compare absolute directory paths without invoking realpath on a missing
$newKeyPath; update the assignment to $inPlaceRotation to first verify existence
of the involved files and then compare their resolved paths (or fallback to
comparing normalized path strings) so a non-existent $newKeyPath does not cause
a false in-place rotation detection.
🧹 Nitpick comments (4)
tests/Data/Settings/Source/EncryptedTest.php (1)
12-16: Add type hints to mock properties.The mock properties lack explicit type hints, which reduces IDE support and type safety.
🔎 Proposed fix
- private $mockFileSystem; - private $mockEncryptor; + private IFileSystem|\PHPUnit\Framework\MockObject\MockObject $mockFileSystem; + private IEncryptor|\PHPUnit\Framework\MockObject\MockObject $mockEncryptor;src/Data/Settings/Source/Encrypted.php (1)
219-231:save()silently swallows all errors.The catch block returns
falsewithout logging or surfacing any error details. This makes debugging save failures difficult. Consider at minimum logging the exception or providing a way to retrieve the last error.🔎 Option: Add error context
catch( \Exception $e ) { + // Consider: error_log( "Failed to save encrypted settings: " . $e->getMessage() ); return false; }Alternatively, rethrow the exception to let callers handle it, matching the
loadSettings()behavior.src/Data/Settings/SecretManager.php (1)
71-74: Editor command execution may not handle editors with arguments.Using
escapeshellcmd()on the entire editor string will escape spaces and special characters, preventing editors specified with arguments (e.g.,"code --wait") from working correctly. Consider usingescapeshellarg()only on the file path.🔎 Alternative approach
// Open in editor - $command = escapeshellcmd( $editor ) . ' ' . escapeshellarg( $tempFile ); + // Don't escape the editor command to allow arguments like "code --wait" + // The temp file path is properly escaped + $command = $editor . ' ' . escapeshellarg( $tempFile );Note: This trades safety for flexibility. If the editor string comes from untrusted input, keep the current approach.
tests/Data/Settings/SecretManagerTest.php (1)
163-169:withConsecutiveis deprecated in PHPUnit 9.6 and removed in PHPUnit 10.The project currently uses PHPUnit 9.x, so this works today. However, the deprecation warning applies now, and the method must be refactored before upgrading to PHPUnit 10. Replace
withConsecutive()calls with callback-based constraints using$this->callback(fn(...$args) => ...)or a reusable helper/trait. Note:willReturnOnConsecutiveCalls()at line 298 still exists in PHPUnit 10 and does not need refactoring.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Data/Settings/SecretManager.phpsrc/Data/Settings/Source/Encrypted.phpsrc/Data/Settings/Source/Memory.phptests/Data/Settings/SecretManagerTest.phptests/Data/Settings/SettingManagerFactoryTest.phptests/Data/Settings/Source/EncryptedTest.php
🧰 Additional context used
🧬 Code graph analysis (5)
tests/Data/Settings/SecretManagerTest.php (1)
src/Data/Settings/SecretManager.php (6)
SecretManager(20-467)generateKey(175-189)show(118-129)encrypt(140-165)validate(198-209)rotateKey(224-390)
src/Data/Settings/Source/Memory.php (3)
src/Data/Settings/SettingManager.php (1)
__construct(28-34)src/Data/Settings/Source/Ini.php (1)
__construct(22-39)src/Data/Settings/Source/Yaml.php (1)
__construct(24-48)
tests/Data/Settings/Source/EncryptedTest.php (1)
src/Data/Settings/Source/Encrypted.php (4)
Encrypted(20-232)get(140-153)getSectionNames(169-172)getSection(190-204)
src/Data/Settings/Source/Encrypted.php (2)
src/Data/Encryption/OpenSSLEncryptor.php (3)
OpenSSLEncryptor(13-228)decrypt(85-139)encrypt(32-75)src/Data/Encryption/IEncryptor.php (2)
decrypt(34-34)encrypt(24-24)
src/Data/Settings/SecretManager.php (4)
src/Data/Encryption/OpenSSLEncryptor.php (4)
OpenSSLEncryptor(13-228)decrypt(85-139)encrypt(32-75)generateKey(147-158)src/Data/Settings/Source/Yaml.php (1)
Yaml(14-123)src/Data/Settings/Source/Encrypted.php (1)
__construct(35-48)src/Data/Encryption/IEncryptor.php (3)
decrypt(34-34)encrypt(24-24)generateKey(42-42)
🪛 Gitleaks (8.30.0)
tests/Data/Settings/Source/EncryptedTest.php
[high] 43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 83-83: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 PHPMD (2.15.0)
tests/Data/Settings/SecretManagerTest.php
139-139: Avoid unused local variables such as '$testKeyPath'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (14)
src/Data/Settings/Source/Memory.php (1)
17-32: LGTM!The constructor provides a clean way to initialize the Memory source with pre-loaded configuration data. The scalar value wrapping logic (
['value' => $settings]) is consistent with the pattern used in theEncryptedsource for handling non-array YAML content.tests/Data/Settings/Source/EncryptedTest.php (2)
41-75: Good test coverage for file-based key reading.The test correctly validates the key retrieval from file and subsequent decryption flow. The Gitleaks warning about "generic-api-key" on line 43 is a false positive—this is clearly test fixture data, not a real credential.
501-521: Well-designed helper method.The
setupMocksForSuccessfulDecryptionhelper effectively reduces duplication across multiple tests while keeping the test setup readable and maintainable.tests/Data/Settings/SecretManagerTest.php (3)
139-147: Static analysis false positive - variable is used in closure.The PHPMD warning about unused
$testKeyPathis incorrect. The variable is captured and used in the closure on line 142 (use ( $plaintextPath, $testKeyPath )).
273-330: Good integration test for key rotation.This test effectively validates the complete key rotation flow using a real filesystem, which is appropriate for testing the atomic file operations. The mock encryptor allows controlled testing of the encryption/decryption steps.
489-524: Thorough testing of secure token generation.The reflection-based test for the private
generateSecureTokenmethod validates cryptographic properties (length, hex format, uniqueness). Testing 100 tokens for uniqueness provides reasonable confidence in the CSPRNG quality.src/Data/Settings/Source/Encrypted.php (2)
81-90: Scalar YAML handling properly addresses the past review concern.The implementation correctly wraps non-array YAML results into a standardized structure (
['value' => ['data' => $parsed]]), preventingTypeErrorexceptions ingetSectionNames()and maintaining consistent array-based access patterns.
190-204: Type safety fix forgetSectionproperly implemented.The method now returns
nullfor scalar section values (lines 198-201), addressing the past review concern about returning type mismatches. This is consistent with thegetSectionSettingNamesvalidation pattern.tests/Data/Settings/SettingManagerFactoryTest.php (3)
46-58: Good regression test for Env singleton fix.This test validates that the factory correctly uses
Env::getInstance()instead of attempting to instantiateEnvdirectly, which would fail due to its private constructor.
100-126: Environment variable priority test is valuable.This test documents and verifies the expected behavior that environment variables take precedence over YAML configuration, which is the correct layered configuration pattern.
179-203: Good coverage forcreateForTestingwith Memory source.The test validates that the Memory constructor enhancement works correctly with the factory's testing mode, enabling fully in-memory configuration for unit tests.
src/Data/Settings/SecretManager.php (3)
47-108:edit()method properly addresses past security concerns.The implementation now:
- Uses
generateSecureToken()for unpredictable temp file names- Sets restrictive permissions (
chmod 0600) immediately after creation- Places file operations inside the
tryblock for proper cleanup viafinally
399-419:readKey()now properly supports environment variable fallback.This addresses the past review concern about inconsistent environment variable support across methods. The
show(),validate(), androtateKey()methods now benefit from this fallback viareadKey().
290-332: Robust atomic rotation with emergency key preservation.The key rotation logic now includes:
- Verification that re-encryption succeeded before committing
- Backup creation before modifying originals
- Emergency key preservation if rollback fails (lines 310-328)
This addresses the past review concern about potential data loss during failed rotations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/Data/Settings/Source/Memory.php (1)
17-32: Consider documenting the scalar wrapping behavior more explicitly.The automatic wrapping of scalar values as
['value' => $settings]is a pragmatic fallback, but callers may not expect that accessing a wrapped scalar requires callingget($section, 'value'). Consider either:
- Adding this detail to the docblock, or
- Throwing an
InvalidArgumentExceptionfor scalar section values if this is not a supported use case🔎 Optional: Enhanced docblock
/** * Constructor * * @param array $config Initial configuration data organized by sections + * Each section value should be an associative array. + * Scalar values are normalized to ['value' => $scalar]. */ public function __construct( array $config = [] )tests/Data/Settings/SecretManagerTest.php (2)
139-147: Unused variable$testKeyPathcaptured in closure.The variable
$testKeyPathis captured in theuseclause but never referenced inside the closure body. The closure only checks$path === $plaintextPath, making$testKeyPathdead code.🔎 Proposed fix
- $testKeyPath = $this->testKeyPath; // Store in local variable for closure $this->mockFileSystem->expects( $this->any() ) ->method( 'fileExists' ) - ->willReturnCallback( function( $path ) use ( $plaintextPath, $testKeyPath ) { + ->willReturnCallback( function( $path ) use ( $plaintextPath ) { if( $path === $plaintextPath ) { return true; } return false; // Key file doesn't exist initially } );
84-84: Consider removing error suppression in tests.Using
@to suppress errors in tests (also at line 171) can hide unexpected issues. If there's a specific warning expected, consider using@expectWarningor capturing the output explicitly.src/Data/Settings/SecretManager.php (3)
71-74: Command injection defense is adequate but consider documenting trust assumptions.The use of
escapeshellcmd()andescapeshellarg()provides protection, but if$editorcan be controlled by untrusted input, edge cases may exist. Consider documenting that the editor parameter should come from trusted configuration only.Additionally, consider validating the editor against an allowlist of known editors if this will be exposed to less-trusted contexts.
182-186: Consider atomic permission setting for key files.The key is written first, then
chmod()is applied. Ifchmod()fails (throws or returns false), the key file remains with default permissions (potentially world-readable). For key files specifically, this timing window is a security concern.🔎 Proposed fix using umask
public function generateKey( string $keyPath, bool $force = false ): string { if( $this->fs->fileExists( $keyPath ) && !$force ) { throw new \Exception( "Key file already exists: $keyPath. Use --force to overwrite." ); } $key = $this->encryptor->generateKey(); + + // Set restrictive umask before writing to ensure file is never world-readable + $oldUmask = umask( 0077 ); + try + { + $this->fs->writeFile( $keyPath, $key ); + } + finally + { + umask( $oldUmask ); + } - $this->fs->writeFile( $keyPath, $key ); // Ensure restrictive permissions (owner read/write only) chmod( $keyPath, 0600 ); return $key; }
276-291: Inconsistent use of nativecopy()vs filesystem abstraction.The
rotateKey()method uses nativecopy()andrename()functions instead of$this->fsmethods throughout (lines 277, 286, 296, 305, 309, etc.). This bypasses the filesystem abstraction, which could complicate testing and break custom filesystem implementations.If
IFileSystemdoesn't provide copy/rename operations, consider extending the interface or documenting that native operations are intentionally used for atomic guarantees.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Data/Settings/SecretManager.phpsrc/Data/Settings/Source/Ini.phpsrc/Data/Settings/Source/Memory.phpsrc/Data/Settings/Source/Yaml.phptests/Data/Settings/SecretManagerTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Data/Settings/Source/Memory.php (1)
src/Data/Settings/SettingManager.php (1)
__construct(28-34)
tests/Data/Settings/SecretManagerTest.php (1)
src/Data/Settings/SecretManager.php (5)
SecretManager(20-517)generateKey(175-189)show(118-129)encrypt(140-165)rotateKey(224-394)
src/Data/Settings/SecretManager.php (5)
src/Data/Encryption/OpenSSLEncryptor.php (4)
OpenSSLEncryptor(13-228)decrypt(85-139)encrypt(32-75)generateKey(147-158)src/Data/Settings/Source/Yaml.php (2)
Yaml(14-134)__construct(24-48)src/Data/Settings/Source/Ini.php (1)
__construct(22-39)src/Data/Settings/Source/Encrypted.php (1)
__construct(35-48)src/Data/Encryption/IEncryptor.php (3)
decrypt(34-34)encrypt(24-24)generateKey(42-42)
🪛 PHPMD (2.15.0)
tests/Data/Settings/SecretManagerTest.php
139-139: Avoid unused local variables such as '$testKeyPath'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
src/Data/Settings/Source/Yaml.php (1)
112-123: LGTM - Defensive guards ensure consistent return type.The checks properly enforce the
?arrayreturn type contract and align with the multi-source merging strategy where sections must be arrays for safearray_mergeoperations.src/Data/Settings/Source/Ini.php (1)
101-112: LGTM - Consistent defensive handling across source implementations.The guards match the pattern in
Yaml.php, ensuring uniform behavior across allISettingSourceimplementations when merging configurations.src/Data/Settings/Source/Memory.php (1)
96-108: LGTM - Consistent defensive handling with good defense-in-depth comment.The comment on lines 101-102 acknowledges that the constructor should prevent this case, but the defensive check protects against future code paths that might bypass normalization (e.g., direct manipulation or subclass overrides).
tests/Data/Settings/SecretManagerTest.php (4)
44-64: LGTM on cleanup implementation.The cleanup method properly handles glob patterns for test artifacts and backup files with appropriate null checks before unlinking.
605-640: Good coverage of cryptographic token generation.The test verifies token length, format, parameterization, and uniqueness across 100 samples. Using reflection to test the private method is a reasonable approach given the security-critical nature of token generation.
415-447: LGTM on environment variable key tests.The tests properly verify fallback to environment variables when key files don't exist, matching the SecretManager's
readKey()behavior. Cleanup viaputenv('NEURON_TEST_KEY')correctly unsets the variable.
163-169:withConsecutive()was deprecated in PHPUnit 9.6 and removed in PHPUnit 10.The codebase currently uses PHPUnit 9.*, so these tests will continue working. However, when upgrading to PHPUnit 10 or later, all four occurrences (lines 165, 294, 514, 568) will need to be replaced with alternatives such as
willReturnOnConsecutiveCalls(), callback-based argument checks, or helper functions.Likely an incorrect or invalid review comment.
src/Data/Settings/SecretManager.php (8)
29-33: LGTM on constructor with dependency injection.Good use of nullable parameters with sensible defaults for the encryptor and filesystem, enabling both production use and testability.
63-69: Temp file creation moved inside try block - properly handles cleanup.The
finallyblock at lines 100-107 ensures the temp file is removed even if exceptions occur during editing or re-encryption. This addresses the previous concern about temp files not being cleaned up.
235-240: Path normalization addresses realpath() issue for non-existent files.The switch from
realpath()tonormalizePath()correctly handles the case where$newKeyPathdoesn't exist yet. This prevents false in-place rotation detection.
250-252: The$credentialsUpdatedWithNewKeytracking variable is now properly used.The variable is set to
trueafter credentials are updated (line 302) and checked during rollback (line 368) to determine whether restoration is needed. This addresses the previous unused variable concern.Also applies to: 301-302, 365-371
481-516: Path normalization handles both Unix and Windows paths.The implementation correctly handles:
- Existing files via
realpath()- Relative paths by prepending
getcwd()- Windows drive letters (line 491)
- Directory normalization with slash handling
The use of native
file_exists()instead of$this->fs->fileExists()is acceptable here since this is for path resolution, not file operations.
403-423: Environment variable fallback properly implemented.The method correctly uses
getenv()(line 416) instead of$_ENV, which ensures reliable environment variable access regardless of PHP'svariables_ordersetting. The check for bothfalseand empty string is appropriate.
455-470: Secure token generation implementation is correct.Uses
random_bytes()for cryptographic randomness with proper exception propagation. The hex encoding ensures filesystem-safe output with predictable length (2× input bytes).
305-336: Emergency key preservation logic handles critical failure scenarios.The nested failure handling at lines 305-336 properly:
- Attempts credentials rollback first
- If rollback fails, preserves the new key to an emergency location
- Uses secure tokens for emergency paths
- Provides clear CRITICAL error messages for manual intervention
This addresses the previous concern about key rotation deleting the new key before credential rollback.
| if( $envValue !== false ) | ||
| { | ||
| return $envValue; | ||
| } |
There was a problem hiding this comment.
Inconsistent empty key handling between Encrypted and SecretManager
Encrypted.getKey() returns an empty string when an environment variable is set to empty (checking only !== false), while SecretManager.readKey() explicitly rejects empty strings (checking !== false && $envValue !== ''). This inconsistency means when a key environment variable like NEURON_MASTER_KEY is set to empty, the Encrypted source silently returns empty settings (the !$key check treats empty string as falsy), while SecretManager throws a clear "Key not found" exception. A user with an encrypted credentials file and an accidentally-empty key env var will have their secrets silently ignored with no indication of misconfiguration.
Additional Locations (1)
1 participant
Note
Adds secure, environment-aware configuration and secret management.
IEncryptor) with OpenSSL implementation (OpenSSLEncryptor) including HMAC and key generationSecretManagerto edit/show/encrypt/validate/rotate encrypted YAML credentials with secure temp files and atomic key rotationEnvironmentDetectorfor consistent env resolution (development,test,staging,production)SettingManagerto support multiple ordered sources; newSettingManagerFactorybuilds stack: base YAML → env YAML → base secrets → env secrets → ENV (highest)Source\Encryptedfor encrypted YAML;Source\Envnow round-trips arrays/objects (JSON), booleans, and null; minor safety checks inIni,Yaml,MemoryVERSIONLOG.md; extensive tests for encryption, secrets, env handling, factory, and sourcesWritten by Cursor Bugbot for commit 97b3097. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Enhancement
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.