[DB-20] feat: secretsManager #265
Conversation
…nto secret-manager-backend
…nto initialization
WalkthroughAdds a secrets management subsystem: storage abstractions ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts`:
- Around line 59-63: In the save method, the try/catch around
writeContentRaw(fsResource, encryptedData.toString("base64")) currently logs the
error and swallows it; change this to propagate the failure so callers know the
save failed (either rethrow the caught err or throw a new Error with context
including fsResource and the original error). Keep the existing logging
(processLogger or console) but after logging rethrow the error so the caller
doesn't assume success; update references in encryptedFsStorage.save where
writeContentRaw, fsResource, and encryptedData are used.
In `@src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts`:
- Line 61: Remove all debug console.log statements that print "!!!debug" in the
FileBasedProviderRegistry implementation (e.g., the one that logs "manifest
loaded" and any others referencing providerManifest); either delete the
console.log calls or replace them with the project logger (e.g., processLogger
or injected logger) at an appropriate level (debug) so no raw console output
remains in production.
- Around line 45-49: The initialize() method calls the async
initProvidersFromManifest() without awaiting it, causing initialize() to resolve
before providers are loaded; update initialize() to await
this.initProvidersFromManifest() so provider loading completes before
initialize() returns (keep existing error propagation/try-catch behavior if
present) and verify getProvider() callers rely on initialize() completion.
- Around line 64-69: In FileBasedProviderRegistry where you iterate
providerManifest and call this.encryptedStorage.load(entry.id), add a null check
after the await and only push non-null SecretProviderConfig values into configs
(skip or remove null results and optionally log a warning/error); ensure the
logic around the configs array and any consumers (e.g., methods that assume
configs entries exist) remains safe by filtering out nulls before further
processing.
In `@src/lib/secretsManager/providerService/AbstractSecretProvider.ts`:
- Line 12: Rename the misspelled abstract method getSecretIdentfier to
getSecretIdentifier in AbstractSecretProvider and update all implementing
classes and callers to use the corrected name; ensure method signatures in
subclasses (e.g., any classes implementing AbstractSecretProvider) and any
references/imports are updated to match the new identifier to avoid breaking
overrides and compile errors.
- Around line 20-22: The abstract methods setSecret and setSecrets in
AbstractSecretProvider lack parameters so callers can't specify which secret(s)
or values to set; change the signatures to accept meaningful inputs (e.g.,
setSecret(secretName: string, value: SecretValue | string, options?:
SetSecretOptions) and setSecrets(secrets: Record<string, SecretValue | string>,
options?: SetSecretsOptions)), then update all concrete
subclasses/implementations to match the new signatures and adjust any
callers/tests to pass the name/value (or map) and optional options.
In `@src/lib/secretsManager/secretsManager.ts`:
- Around line 11-13: The initialize method in secretsManager (async
initialize()) calls this.registry.initialize() without awaiting it, causing the
method to resolve before registry setup completes; update the method to await
this.registry.initialize() (i.e., await this.registry.initialize();) so the
Promise is properly awaited and initialization completes before returning.
- Around line 20-22: The removeProviderConfig method calls an async
registry.deleteProviderConfig(id) without awaiting it; update
removeProviderConfig (async removeProviderConfig) to await the call — e.g.,
await this.registry.deleteProviderConfig(id) — and return its result or let any
thrown error propagate to the caller so callers can observe success/failure
(ensure the unique symbol this.registry.deleteProviderConfig is awaited and its
promise handled).
- Around line 15-18: The addProviderConfig function currently logs a debug
message and calls registry.setProviderConfig(config) without awaiting it; remove
the console.log("!!!debug", "addconfig", config) line and change the call to
await this.registry.setProviderConfig(config) so the async operation errors
propagate to callers and the caller can observe success/failure; target the
addProviderConfig method and the registry.setProviderConfig invocation for this
change.
- Around line 28-31: The testProviderConnection method is returning a Promise
instead of a boolean because provider?.testConnection() is not awaited; update
testProviderConnection to await the provider.testConnection() call (use the
provider from this.registry.getProvider(id) and if provider is undefined return
false) so the method resolves to a boolean as promised; reference the
testProviderConnection function, registry.getProvider(id), and
provider.testConnection() when making the change.
In `@src/lib/secretsManager/types.ts`:
- Around line 5-10: Remove the debug console.log statements that print full
credential objects (look for logs with "!!!debug" and any console.log of
SecretProviderConfig or AWSSecretsManagerConfig) so secrets are not exposed;
specifically delete the console.log that prints the SecretProviderConfig in
secretsManager.ts and the one in FileBasedProviderRegistry (class
FileBasedProviderRegistry) that logs provider configs, or replace them with
safe/redacted logging that omits or masks accessKeyId, secretAccessKey and
sessionToken (e.g., log only non-sensitive metadata or use a secure logger and
explicit redaction before logging).
In `@src/main/events.js`:
- Around line 301-304: The IPC handler
ipcMain.handle("secretsManager:getProviderConfig", async (event, id) => { ... })
currently logs the result and doesn't return it to the caller; remove the debug
console.log("!!!debug", "getConfig", providerConfig) and return providerConfig
(or an appropriate serializable object) from the handler so the IPC caller
receives the data; keep the async signature and propagate or handle errors as
needed (e.g., throw or return an error payload) but ensure the handler returns
the providerConfig value.
- Around line 277-295: The IPC handlers risk a null reference because the
module-level variable secretsManager is only set in the "init-secretsManager"
handler; update the other handlers (addProviderConfig, getProviderConfig,
removeProviderConfig) to first check that secretsManager is non-null and either
return a clear error/rejection to the caller or lazily initialize it (e.g., call
the same initialization logic used in the "init-secretsManager" handler) before
using it; ensure the init handler returns a success/failure result and that all
uses of secretsManager guard against null to avoid runtime exceptions.
In `@src/renderer/actions/local-sync/schemas.ts`:
- Around line 204-210: Update the schema for the "providers" field to reflect it
is optional: change the current providers: Type.Array(...) to providers:
Type.Optional(Type.Array(...)) so it aligns with the migration and load logic
that treat providers as optional/defaults; also tighten the provider "type"
element by replacing Type.String() with a discriminated set (e.g., Type.Union of
Type.Literal values like "aws", "gcp", etc.) to enforce valid provider types,
keeping the surrounding object shape (id: Type.String()) the same; modify the
providers declaration in the schema where the providers property is defined to
implement these changes.
🧹 Nitpick comments (7)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (1)
62-62: Remove debug artifact from log message.The
!!!debugprefix appears to be leftover debugging code that should be cleaned up before merging.src/lib/secretsManager/providerService/AbstractSecretProvider.ts (2)
10-10: Consider stronger typing forconfig.Using
anyloses type safety. Consider using a generic type parameter or a base configuration interface to preserve type information across provider implementations.♻️ Example with generics
-export abstract class AbstractSecretProvider { +export abstract class AbstractSecretProvider<TConfig = unknown> { protected cache: Map<string, CachedSecret> = new Map(); abstract readonly type: SecretProviderType; abstract readonly id: string; - protected config: any; + protected config: TConfig;
24-26: StaticvalidateConfigthrows unconditionally - document expected override pattern.Since TypeScript doesn't support abstract static methods, throwing "Not implemented" is a reasonable workaround. However, consider adding a JSDoc comment to clarify that subclasses should override this method, or use a different pattern like a separate validation function per provider type.
src/main/events.js (1)
297-308: Consider adding error handling to IPC handlers.The
addProviderConfigandremoveProviderConfighandlers don't wrap operations in try/catch. Unhandled exceptions in IPC handlers can be difficult to debug from the renderer side.♻️ Suggested pattern for consistent error handling
ipcMain.handle("secretsManager:addProviderConfig", async (event, config) => { + try { await secretsManager.addProviderConfig(config); + } catch (err) { + console.error("Error adding provider config", err); + throw err; + } });src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts (2)
33-33: Redundant redeclaration of inherited property.The
providersMap is already declared inAbstractProviderRegistry(line 15 of parent class). This redeclaration shadows the parent's property unnecessarily.Proposed fix
private configDir: string; - protected providers: Map<string, AbstractSecretProvider> = new Map(); - constructor(encryptedStorage: AbstractEncryptedStorage, configDir: string) {
26-27: Address TODO comments or create tracking issues.There are TODO comments regarding version checking (line 26) and schema handling (line 183). These should either be addressed in this PR or tracked as follow-up issues.
Would you like me to help create GitHub issues to track these TODOs?
Also applies to: 183-184
src/lib/secretsManager/types.ts (1)
31-40: Consider memory security for cached secret values.
CachedSecret.valuestores the plaintext secret in memory. This is acceptable for runtime use, but consider:
- Implementing a cleanup mechanism when secrets expire
- Avoiding logging or serializing
CachedSecretobjects with thevaluefield
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/lib/secretsManager/encryptedStorage/AbstractEncryptedStorage.tssrc/lib/secretsManager/encryptedStorage/encryptedFsStorage.tssrc/lib/secretsManager/providerRegistry/AbstractProviderRegistry.tssrc/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.tssrc/lib/secretsManager/providerService/AbstractSecretProvider.tssrc/lib/secretsManager/providerService/awsSecretManagerProvider.tssrc/lib/secretsManager/providerService/providerFactory.tssrc/lib/secretsManager/secretsManager.tssrc/lib/secretsManager/types.tssrc/main/events.jssrc/renderer/actions/local-sync/constants.tssrc/renderer/actions/local-sync/file-types/file-types.tssrc/renderer/actions/local-sync/fs-utils.tssrc/renderer/actions/local-sync/schemas.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/lib/secretsManager/providerService/providerFactory.ts (2)
src/lib/secretsManager/types.ts (1)
SecretProviderConfig(14-21)src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
AWSSecretsManagerProvider(4-4)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (2)
src/renderer/actions/local-sync/common-utils.ts (2)
createFsResource(32-83)appendPath(27-30)src/renderer/actions/local-sync/fs-utils.ts (5)
getIfFolderExists(84-89)createFolder(144-189)writeContentRaw(315-351)parseFileRaw(416-429)deleteFsResource(98-142)
src/renderer/actions/local-sync/file-types/file-types.ts (1)
src/renderer/actions/local-sync/schemas.ts (6)
ApiRecord(175-178)EnvironmentRecord(190-193)Variables(180-188)Description(126-126)Auth(102-124)GlobalConfig(195-211)
src/lib/secretsManager/providerService/AbstractSecretProvider.ts (1)
src/lib/secretsManager/types.ts (2)
CachedSecret(31-40)SecretReference(29-29)
src/lib/secretsManager/secretsManager.ts (1)
src/lib/secretsManager/types.ts (1)
SecretProviderConfig(14-21)
src/lib/secretsManager/encryptedStorage/AbstractEncryptedStorage.ts (1)
src/main/actions/networkSessionStorage/index.js (1)
data(102-102)
src/renderer/actions/local-sync/fs-utils.ts (3)
src/renderer/actions/local-sync/common-utils.ts (1)
getNormalizedPath(22-25)src/renderer/actions/local-sync/constants.ts (1)
CORE_CONFIG_FILE_VERSION(3-3)src/renderer/actions/local-sync/schemas.ts (1)
GlobalConfig(195-211)
src/lib/secretsManager/providerRegistry/AbstractProviderRegistry.ts (1)
src/lib/secretsManager/types.ts (1)
SecretProviderConfig(14-21)
🔇 Additional comments (12)
src/renderer/actions/local-sync/constants.ts (1)
3-3: LGTM!Version bump to 0.2 appropriately reflects the schema change with the new
providersfield in GlobalConfig.src/lib/secretsManager/encryptedStorage/AbstractEncryptedStorage.ts (1)
1-12: LGTM!Clean abstract class design establishing a clear contract for encrypted storage implementations. The generic type constraints and async method signatures are appropriate for the use case.
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (1)
3-13: No action required. The imported utilities (common-utilsandfs-utils) use only Node.js core APIs (fs, fs/promises, path) and contain no renderer-specific code. These utilities are process-agnostic and work correctly when imported into main process code likeelectron.safeStorage. While the utilities are located in therenderer/directory, this is a code organization preference rather than a functional issue.src/renderer/actions/local-sync/fs-utils.ts (5)
431-468: LGTM! ExportedwriteToGlobalConfigfor external use.The export enables other modules (like SecretsManager) to persist global config changes. The implementation correctly ensures the config folder exists before writing, uses elevated access, and returns appropriate error types.
527-541: LGTM! Proper initialization ofprovidersfield for new configs.When creating a new global config file, the
providersarray is correctly initialized as empty. This aligns with the schema definition inschemas.ts.
552-556: Good use of spread operator to preserve existing fields.The spread
...readResult.contentensuresprovidersand any other existing fields are carried forward when adding a workspace to an existing config.
614-632: LGTM! Migration logic handles version transitions correctly.The migration properly handles:
- Legacy configs without a version field (pre-versioning)
- v0.1 configs that need the
providersfield addedBoth cases correctly initialize
providers: []and bump to the current version.
741-746: Correct preservation ofprovidersduring workspace pruning.The
providersfield is preserved with a fallback to empty array (|| []) when reconstructing the config after pruning invalid workspaces.src/renderer/actions/local-sync/file-types/file-types.ts (2)
15-23: LGTM! Explicit type annotations improve type safety.The explicit
validator: typeof ApiRecord = ApiRecordpattern is clearer and provides better TypeScript inference compared to implicit property initialization.
25-77: Consistent type annotation pattern applied across all FileType classes.All six FileType subclasses now use the same explicit type annotation pattern for their
validatorproperties, improving code consistency and maintainability.src/lib/secretsManager/providerService/providerFactory.ts (1)
5-14: LGTM! Clean factory pattern for provider instantiation.The factory is well-structured and extensible. The error message helpfully includes the unknown type value for debugging. As more provider types are added, this switch statement can be extended.
Note: This depends on
AWSSecretsManagerProviderwhich is currently a skeleton implementation (see review comment on that file).src/lib/secretsManager/providerRegistry/AbstractProviderRegistry.ts (1)
12-36: LGTM!The abstract class design provides a clean contract for provider registry implementations. Good use of protected methods for manifest handling and appropriate separation between sync (
getProviderfor in-memory lookup) and async operations (storage/config operations).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts
Outdated
Show resolved
Hide resolved
src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Initialization of class' singleton instance has been done in PR: #266
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts`:
- Around line 36-59: The save implementation (and corresponding load/delete
methods) must sanitize the provided key to prevent path traversal: add a private
sanitizeKey(key: string) helper and call it at the start of save, load, and
delete to reject keys containing "..", "/" or "\" (throw a clear Error like
`Invalid storage key: ${key}`) and return the cleaned key; then use that
sanitized key when building fsResource with appendPath(this.baseFolderPath,
sanitizedKey) and when creating FsResource or checking providerFolderExists so
all file operations (createFsResource, appendPath, getIfFolderExists,
createFolder) operate inside baseFolderPath only.
In `@src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts`:
- Around line 120-222:
ensureConfigDir/ensureConfigFile/loadManifest/saveManifest are inconsistent:
loadManifest and ensureConfigFile use this.configDir/createFsResource, but
createGlobalConfigFolder() and writeToGlobalConfig() use the hardcoded
GLOBAL_CONFIG_FOLDER_PATH. Update the calls so all file operations respect the
instance configDir (and manifestPath) — either by passing this.configDir (and
this.manifestPath when needed) into createGlobalConfigFolder and
writeToGlobalConfig, or by refactoring those helpers to accept a target path;
ensure saveManifest uses the same createFsResource(this.configDir,
this.manifestPath) path flow as loadManifest and that errors/logs reference the
same resource functions (functions to edit: ensureConfigDir, ensureConfigFile,
loadManifest, saveManifest and helpers createGlobalConfigFolder,
writeToGlobalConfig).
♻️ Duplicate comments (2)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (1)
61-65: Don’t swallow write failures insave.
This still logs and continues, which can silently drop configs. (Duplicate of prior feedback.)🐛 Proposed fix
try { await writeContentRaw(fsResource, encryptedData.toString("base64")); } catch (err) { - console.error("!!!debug", "Error writing encrypted data", err); + console.error("Error writing encrypted data", err); + throw err; }src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts (1)
59-79: Removeconsole.log("!!!debug")before merge.
Please drop or replace with the project logger at debug level. (Duplicate of prior feedback.)🧹 Suggested cleanup
- console.log("!!!debug", "manifest loaded", providerManifest); @@ - console.log("!!!debug", "Config not found for entry", entry); @@ - console.log("!!!debug", "all configs", configs); @@ - console.log("!!!debug", "readResult", readResult);Also applies to: 189-190
src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts`:
- Around line 187-194: loadManifest currently parses the file and casts the
result to ProviderManifest (an array) but the file is written as a GlobalConfig
object by ensureConfigFile/saveManifest; change loadManifest to parse into a
GlobalConfig, then return the providers array (manifest.providers) or an empty
array if missing, and adjust the type assertion accordingly (use GlobalConfig
for parsed object and only cast the returned value to ProviderManifest[]);
ensure error handling remains for invalid JSON.
♻️ Duplicate comments (1)
src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts (1)
59-80: Remove debug console.log statements before merging.Multiple
!!!debuglog statements remain throughout the file (lines 62, 74, 78, 185). These should be removed or replaced with a proper logging utility at an appropriate log level.
🧹 Nitpick comments (1)
src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts (1)
10-23: Consider extracting shared utilities to avoid lib → renderer dependency.The
libdirectory typically contains shared, process-agnostic code, but these imports create a coupling to renderer-specific modules. Iflibcode is later consumed by the main process directly (outside IPC), this dependency graph could cause issues.Consider either:
- Moving the file system utilities (
createFsResource,fs-utils, etc.) to a shared location underlib/- Or keeping this registry implementation in the renderer layer
src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts`:
- Around line 121-129: The delete method in encryptedFsStorage.ts currently
ignores the result of deleteFsResource which returns a FileSystemResult, leading
to false success; update the async delete(key: string) implementation
(referencing sanitizeKey, createFsResource, appendPath, deleteFsResource) to
await and inspect the returned FileSystemResult, and if it indicates an error or
failure, throw or return a rejected Promise with a clear error (including the
key and result details) so callers see the deletion failure instead of assuming
success.
- Around line 16-33: The sanitizeKey function currently rewrites unsafe keys
(replacing ".." and path separators) which can cause distinct keys to collide;
change sanitizeKey to validate and reject unsafe keys instead of transforming
them by checking for forbidden patterns (e.g., any occurrences of "..", any path
separators "/" or "\", leading dots, or other disallowed characters) and
throwing descriptive errors when such patterns are found so callers must provide
a safe, 1:1-mapped key; keep the existing checks for empty/whitespace-only input
and return the original key unchanged when it passes validation.
♻️ Duplicate comments (1)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (1)
55-86: Propagate folder/write failures (currently silently ignored).
createFolderandwriteContentRawreturn aFileSystemResultand don’t throw; the current try/catch won’t catch failures and the method returns success on error.🔧 Suggested fix
- if (!providerFolderExists) { - await createFolder(fsFolderResource); - } + if (!providerFolderExists) { + const createResult = await createFolder(fsFolderResource); + if (createResult.type === "error") { + throw new Error( + `Failed to create storage folder: ${createResult.error.message}` + ); + } + } @@ - try { - await writeContentRaw(fsResource, encryptedData.toString("base64")); - } catch (err) { - console.error("!!!debug", "Error writing encrypted data", err); - } + const writeResult = await writeContentRaw( + fsResource, + encryptedData.toString("base64") + ); + if (writeResult.type === "error") { + console.error("Error writing encrypted data", writeResult.error); + throw new Error( + `Failed to write encrypted data: ${writeResult.error.message}` + ); + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts`:
- Around line 69-73: The code calls createFolder(fsFolderResource) but ignores
its FileSystemResult return; update the block so that after calling createFolder
you check the returned FileSystemResult (from createFolder) and if it indicates
failure (e.g., success flag false or an error field set) log/throw an
informative error including the result details and abort further operations (so
later writes don't run on a failed create); reference getIfFolderExists,
createFolder, and fsFolderResource when locating and updating this logic.
♻️ Duplicate comments (1)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (1)
81-85: Critical: Silent failure - error is caught but not re-thrown.The
savemethod catches write errors but only logs them, causing silent data loss. The caller will believe the save succeeded when it actually failed. Additionally, the"!!!debug"prefix appears to be a debug artifact that should be removed.🐛 Proposed fix
try { await writeContentRaw(fsResource, encryptedData.toString("base64")); } catch (err) { - console.error("!!!debug", "Error writing encrypted data", err); + console.error("Error writing encrypted data", err); + throw err; }
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/providerRegistry/FileBasedProviderRegistry.ts`:
- Around line 11-15: Wrap each call to createProviderInstance inside a try/catch
so a thrown error for one config doesn’t abort initProvidersFromStorage; in
initProvidersFromStorage iterate configs, attempt const instance =
createProviderInstance(config) in try, only call this.providers.set(config.id,
instance) on success, and log/collect the error for that config otherwise. Apply
the same pattern in setProviderConfig: instantiate via
createProviderInstance(config) before persisting the config, and only
write/update storage and this.providers when instantiation succeeds; on failure
do not persist and surface or log the error. Ensure you reference
createProviderInstance, initProvidersFromStorage, setProviderConfig,
getAllProviderConfigs and the providers Map in the changes.
In `@src/lib/storage/EncryptedElectronStore.ts`:
- Around line 21-26: The EncryptedElectronStore constructor can throw when
safeStorage.isEncryptionAvailable() is false, and the call site new
SecretsManagerEncryptedStorage("providers") (in events.js) is not wrapped,
causing uncaught crashes; fix by either wrapping the instantiation of
SecretsManagerEncryptedStorage in a try-catch where it's created (handle/log the
error and disable secrets features or return early) or make
SecretsManagerEncryptedStorage's constructor defensive by catching the
EncryptedElectronStore construction error and falling back to a
non-encrypted/no-op store; update the call site around new
SecretsManagerEncryptedStorage("providers") and ensure
secretsManager.initialize() is only called when initialization succeeded.
In `@src/main/events.js`:
- Line 28: The import for SecretsManagerEncryptedStorage uses a bare "lib/..."
path causing Node to resolve it as a package; update the import to the same
relative style as other imports (prefix with ../) so that
SecretsManagerEncryptedStorage is imported via the relative path (e.g.,
../lib/secretsManager/encryptedStorage/SecretsManagerEncryptedStorage) to fix
module resolution errors.
- Around line 283-287: The call to secretsManager.initialize() is async but not
awaited, so move to await the promise and handle errors: update the surrounding
function (e.g., the event handler or init function containing the try/catch) to
be async if it isn't, then use await secretsManager.initialize() inside the
existing try/catch so rejected promises are caught and initialization completes
before subsequent IPC calls; alternatively, if you cannot make the wrapper
async, attach .catch(...) to secretsManager.initialize() to handle errors and
prevent race conditions.
♻️ Duplicate comments (1)
src/main/events.js (1)
290-301: Null checks missing andgetProviderConfigdoesn't return a value.Per earlier feedback (acknowledged as test code to be fixed before merge):
- If any handler is called before
init-secretsManager, accessingsecretsManagerwill throw a null reference errorgetProviderConfig(line 295) retrieves the config but doesn't return it to the IPC caller- Debug
console.log("!!!debug", ...)should be removed
🧹 Nitpick comments (1)
src/lib/secretsManager/encryptedStorage/SecretsManagerEncryptedStorage.ts (1)
21-23: Fix getAll typing to avoid lossy Object.values.
getAll<SecretProviderConfig>()is typed as a single config, soObject.valuesbecomes field values. Use a record type (or adjustEncryptedElectronStore.getAllto returnRecord<string, T>).♻️ Proposed fix
- async getAll(): Promise<SecretProviderConfig[]> { - const allData = this.encryptedStore.getAll<SecretProviderConfig>(); - return Object.values(allData); - } + async getAll(): Promise<SecretProviderConfig[]> { + const allData = + this.encryptedStore.getAll<Record<string, SecretProviderConfig>>(); + return Object.values(allData); + }
Summary by CodeRabbit
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.