[DB-20] feat: secretsManager #265
Conversation
…nto secret-manager-backend
…nto initialization
WalkthroughAdds a secrets-management subsystem: abstract provider and storage base classes, a file-backed provider registry, concrete encrypted storage (EncryptedElectronStore and SecretsManagerEncryptedStorage), a SecretsManager singleton coordinating providers, provider factory, AWS Secrets Manager and HashiCorp Vault provider modules (with typed interfaces and caching), comprehensive types mapping provider types to credentials/references/values, IPC bindings in main/events, and usage examples. The changes introduce runtime implementations, type-safe helpers, and integration points for persisting and retrieving provider configurations. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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
🧪 Generate unit tests (beta)
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
src/lib/secretsManager/providerService/AbstractSecretProvider.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); + }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/providerService/awsSecretManagerProvider.ts`:
- Around line 50-52: Remove all debug console.log statements that print secret
contents or full AWS responses (e.g., statements logging "aws result", the `res`
object, or `data`/`SecretString`) in the AWS Secrets Manager provider; instead
log only non-sensitive, high-level status information (e.g., success/failure and
sanitized HTTP status) and never include `SecretString`, `SecretBinary` or full
`$metadata`. Locate usages of `res`, `data`, and any `console.log` calls inside
the secret retrieval function (references: `res`, `data`,
`$metadata.httpStatusCode`) and either delete those debug logs or replace them
with redacted messages that omit secret material and only show safe fields like
`res.$metadata.httpStatusCode` (if necessary) after explicit redaction.
In `@src/lib/secretsManager/types.ts`:
- Around line 7-12: The AWSSecretsManagerConfig type should keep
accessKeyId/secretAccessKey/sessionToken optional, but the constructor that
creates the SecretsManagerClient must not pass a credentials object with
undefined values; instead, when accessKeyId/secretAccessKey are not provided,
omit the credentials property entirely from the client config so the AWS default
provider chain can be used (update the code around the
SecretsManagerClient/constructor where the client is created). Also update
validateConfig to treat credentials as optional (only region required) and
accept undefined accessKeyId/secretAccessKey/sessionToken so it no longer
enforces creating an empty credentials object.
In `@src/main/events.js`:
- Around line 324-330: Remove the console.log that prints the resolved secret
value to avoid leaking secrets: specifically stop logging the variable
secretValue returned by secretsManager.getSecret in the block that currently
logs "resolve" and "resolveSecret value"; instead, log only non-sensitive
identifiers like providerId and ref (or a success/failure message) and ensure
secretsManager.getSecret's return (secretValue) is never written to logs or
error messages in the functions referencing secretValue.
| console.log("!!!debug", "aws result", res); | ||
|
|
||
| if (res.$metadata.httpStatusCode !== 200) { |
There was a problem hiding this comment.
Remove debug logs that can expose secret material or sensitive metadata.
Several logs print secret values or full responses, which can leak secrets via logs.
🛡️ Proposed redaction/removal
- console.log("!!!debug", "aws result", res);
+ // Optionally log minimal metadata only, or remove entirely.
- console.error(
- "!!!debug",
- "aws secrets manager test connection error",
- err
- );
+ console.error("AWS Secrets Manager test connection error", err);
- console.log("!!!debug", "returning from cache", cachedSecret);
+ // Avoid logging cached secret values.
- console.error("!!!debug", "Failed to fetch secret", secretResponse);
+ console.error("AWS Secrets Manager: failed to fetch secret", {
+ httpStatusCode: secretResponse.$metadata?.httpStatusCode,
+ requestId: secretResponse.$metadata?.requestId,
+ });
- console.error("!!!debug", "SecretString is empty", secretResponse);
+ console.error("AWS Secrets Manager: SecretString is empty", {
+ requestId: secretResponse.$metadata?.requestId,
+ });
- console.log("!!!debug", "returning after fetching", awsSecret);
+ // Avoid logging fetched secret values.Also applies to: 58-62, 75-77, 88-94, 107-108
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/providerService/awsSecretManagerProvider.ts` around
lines 50 - 52, Remove all debug console.log statements that print secret
contents or full AWS responses (e.g., statements logging "aws result", the `res`
object, or `data`/`SecretString`) in the AWS Secrets Manager provider; instead
log only non-sensitive, high-level status information (e.g., success/failure and
sanitized HTTP status) and never include `SecretString`, `SecretBinary` or full
`$metadata`. Locate usages of `res`, `data`, and any `console.log` calls inside
the secret retrieval function (references: `res`, `data`,
`$metadata.httpStatusCode`) and either delete those debug logs or replace them
with redacted messages that omit secret material and only show safe fields like
`res.$metadata.httpStatusCode` (if necessary) after explicit redaction.
src/lib/secretsManager/types.ts
Outdated
| export interface AWSSecretsManagerConfig { | ||
| accessKeyId: string; | ||
| secretAccessKey: string; | ||
| region: string; | ||
| sessionToken?: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate the secrets manager implementation
find src -type f -name "*.ts" | grep -i secret | head -20Repository: requestly/requestly-desktop-app
Length of output: 621
🏁 Script executed:
# Check the types file and surrounding code
cat -n src/lib/secretsManager/types.tsRepository: requestly/requestly-desktop-app
Length of output: 1692
🏁 Script executed:
# Search for how AWSSecretsManagerConfig is used throughout the codebase
rg "AWSSecretsManagerConfig" --type ts -A 3 -B 1Repository: requestly/requestly-desktop-app
Length of output: 2594
🏁 Script executed:
# Find the secrets manager client implementation to see how config is used
find src -type f -name "*secret*" -o -name "*Secrets*" | head -20Repository: requestly/requestly-desktop-app
Length of output: 262
🏁 Script executed:
# Get the full implementation of the AWS provider
cat -n src/lib/secretsManager/providerService/awsSecretManagerProvider.tsRepository: requestly/requestly-desktop-app
Length of output: 5414
🏁 Script executed:
# Also check the abstract provider to understand the initialization pattern
cat -n src/lib/secretsManager/providerService/AbstractSecretProvider.tsRepository: requestly/requestly-desktop-app
Length of output: 3210
🌐 Web query:
AWS SDK v3 SecretsManagerClient default credential provider chain credentials parameter omitted
💡 Result:
If you omit the credentials (or credentialsProvider) option when constructing SecretsManagerClient in AWS SDK for JavaScript v3, the client automatically uses the default credential provider chain (Node.js only). (docs.aws.amazon.com)
What it tries (Node.js), in precedence order
The v3 default provider (defaultProvider) resolves credentials by checking, in order: (docs.aws.amazon.com)
- Environment variables (for example
AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY,AWS_SESSION_TOKEN) - AWS IAM Identity Center (SSO) cached credentials (if configured via AWS CLI)
- Web identity token credentials (for example IRSA on EKS via
AWS_WEB_IDENTITY_TOKEN_FILE+ role ARN) - Shared config/credentials files (
~/.aws/credentials,~/.aws/config, honoringAWS_PROFILE) - Container/instance metadata (ECS task role / EC2 instance profile via metadata services)
It stops at the first source that yields valid credentials; if none work, requests fail with a “unable to load credentials” type error. (docs.aws.amazon.com)
Important runtime note
In browser/React Native runtimes, the default credential chain is effectively empty, so you generally must provide credentials explicitly (or implement your own provider). (docs.aws.amazon.com)
Citations:
- 1: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrate-credential-providers.html?utm_source=openai
- 2: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrate-credential-providers.html?utm_source=openai
- 3: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrate-credential-providers.html?utm_source=openai
- 4: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrate-credential-providers.html?utm_source=openai
🏁 Script executed:
# Check if this is an Electron app and what framework is used
cat -n package.json | head -50Repository: requestly/requestly-desktop-app
Length of output: 2960
🏁 Script executed:
# Also check if there's any electron config or main process file
fd -e json -e ts -e js | grep -i "electron\|main" | head -20Repository: requestly/requestly-desktop-app
Length of output: 769
Type change alone is insufficient—implementation must conditionally omit credentials to enable AWS default provider chain.
Making the fields optional in the type definition won't work as proposed. The constructor currently creates an explicit credentials object that would still be passed to SecretsManagerClient (with undefined values), which prevents the default provider chain from activating.
To support the default chain, you must:
- Keep the type changes as suggested.
- Modify the constructor to conditionally omit the
credentialsproperty entirely when not provided:
♻️ Required implementation changes
constructor(providerConfig: SecretProviderConfig) {
super();
this.id = providerConfig.id;
this.config = providerConfig.config as AWSSecretsManagerConfig;
- this.client = new SecretsManagerClient({
+ const clientConfig: any = {
region: this.config.region,
+ ...(this.config.accessKeyId &&
+ this.config.secretAccessKey && {
+ credentials: {
+ accessKeyId: this.config.accessKeyId,
+ secretAccessKey: this.config.secretAccessKey,
+ sessionToken: this.config.sessionToken,
+ },
+ }),
- credentials: {
- accessKeyId: this.config.accessKeyId,
- secretAccessKey: this.config.secretAccessKey,
- sessionToken: this.config.sessionToken,
- },
- });
+ };
+ this.client = new SecretsManagerClient(clientConfig);
}- Update
validateConfigto allow undefined credentials (since region becomes the only required field).
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/types.ts` around lines 7 - 12, The
AWSSecretsManagerConfig type should keep
accessKeyId/secretAccessKey/sessionToken optional, but the constructor that
creates the SecretsManagerClient must not pass a credentials object with
undefined values; instead, when accessKeyId/secretAccessKey are not provided,
omit the credentials property entirely from the client config so the AWS default
provider chain can be used (update the code around the
SecretsManagerClient/constructor where the client is created). Also update
validateConfig to treat credentials as optional (only region required) and
accept undefined accessKeyId/secretAccessKey/sessionToken so it no longer
enforces creating an empty credentials object.
| console.log("!!!debug", "resolve", { | ||
| providerId, | ||
| ref, | ||
| }); | ||
| const secretValue = await secretsManager.getSecret(providerId, ref); | ||
| console.log("!!!debug", "resolveSecret value", secretValue); | ||
| return secretValue; |
There was a problem hiding this comment.
Do not log resolved secrets (secret values leak).
secretValue includes the secret string; logging it is a critical leak vector.
🛡️ Proposed fix
ipcMain.handle(
"secretsManager:resolveSecret",
async (event, { providerId, ref }) => {
- console.log("!!!debug", "resolve", {
- providerId,
- ref,
- });
const secretValue = await secretsManager.getSecret(providerId, ref);
- console.log("!!!debug", "resolveSecret value", secretValue);
return secretValue;
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("!!!debug", "resolve", { | |
| providerId, | |
| ref, | |
| }); | |
| const secretValue = await secretsManager.getSecret(providerId, ref); | |
| console.log("!!!debug", "resolveSecret value", secretValue); | |
| return secretValue; | |
| const secretValue = await secretsManager.getSecret(providerId, ref); | |
| return secretValue; |
🤖 Prompt for AI Agents
In `@src/main/events.js` around lines 324 - 330, Remove the console.log that
prints the resolved secret value to avoid leaking secrets: specifically stop
logging the variable secretValue returned by secretsManager.getSecret in the
block that currently logs "resolve" and "resolveSecret value"; instead, log only
non-sensitive identifiers like providerId and ref (or a success/failure message)
and ensure secretsManager.getSecret's return (secretValue) is never written to
logs or error messages in the functions referencing secretValue.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts`:
- Around line 181-217: The example5 function uses
AWSSecretsManagerProvider.setSecret/setSecrets and
HashicorpVaultProvider.setSecret/setSecrets which currently throw "Method not
implemented."; update the example to avoid invoking unimplemented write APIs by
either (a) replacing those calls with commented lines or a clear "not supported
yet" note, or (b) wrapping each call in a try/catch that logs/ignores the
NotImplemented error, and keep the rest of example5 intact; reference the
AWSSecretsManagerProvider, HashicorpVaultProvider, setSecret, setSecrets, and
example5 symbols when making the change so readers understand these writes are
not yet implemented.
- Around line 8-17: The import of createTypedProviderInstance in
USAGE_EXAMPLES.ts fails because providerFactory does not export it; add a typed
helper export in providerFactory that wraps createProviderInstance: declare and
export createTypedProviderInstance<T extends SecretProviderType>(config:
ProviderConfigForProvider<T>) returning AbstractSecretProvider<T> and implement
it by delegating to createProviderInstance(config) and casting the result to
AbstractSecretProvider<T>; this ensures the named export exists and preserves
the typed API used by USAGE_EXAMPLES.ts.
🧹 Nitpick comments (5)
src/lib/secretsManager/providerService/hashicorpVaultProvider.ts (2)
21-34: Consider removing the redundantpathfield.The
pathfield on line 26 duplicatessecretReference.path(sinceVaultSecretReferencealready containspath). This redundancy could lead to inconsistencies if the values diverge.♻️ Suggested change
export interface VaultSecretValue { type: SecretProviderType.HASHICORP_VAULT; providerId: string; secretReference: VaultSecretReference; fetchedAt: number; - path: string; data: Record<string, any>; metadata?: { version: number; created_time: string; deletion_time?: string; destroyed?: boolean; }; }
53-93: Stub methods are acceptable for initial scaffolding.All methods correctly throw
Error("Method not implemented.")which is appropriate for a placeholder. ThevalidateConfigprovides minimal validation by checking for the requiredaddressfield.Consider adding a TODO comment or tracking issue to implement the actual Vault integration.
Would you like me to open an issue to track the implementation of the HashiCorp Vault provider methods?
src/lib/secretsManager/providerService/AbstractSecretProvider.ts (3)
54-60: Consider deleting expired entries ingetCachedSecret.When an expired entry is found, it's not removed from the cache. This means stale entries persist until
setCacheEntryorevictExpiredEntriesis called. Consider deleting the expired entry immediately.♻️ Suggested change
protected getCachedSecret(key: string): ValueForProvider<T> | null { const cached = this.cache.get(key); if (cached && cached.fetchedAt + this.cacheTtlMs > Date.now()) { return cached; } + if (cached) { + this.cache.delete(key); + } return null; }
80-91: Fix implicit return in forEach callback.The static analysis tool correctly flags that the arrow function implicitly returns the result of
this.cache.delete(key). While this doesn't cause a bug, it's cleaner to be explicit.♻️ Suggested fix
- keysToDelete.forEach((key) => this.cache.delete(key)); + keysToDelete.forEach((key) => { + this.cache.delete(key); + });
95-103: The!configcheck is redundant.Since the method returns
falseunconditionally on line 102, the early return on lines 98-100 has no effect on the outcome. Consider simplifying or adding documentation that this is intentional fail-safe behavior.♻️ Simplified version
static validateConfig(config: any): boolean { // Base implementation rejects all configs as a fail-safe. // Provider implementations must override with specific validation. - if (!config) { - return false; - } - + void config; // Unused - subclasses must override return false; }
| import { | ||
| SecretProviderType, | ||
| AWSSecretProviderConfig, | ||
| HashicorpVaultProviderConfig, | ||
| AwsSecretReference, | ||
| VaultSecretReference, | ||
| } from "./types"; | ||
| import { createProviderInstance, createTypedProviderInstance } from "./providerService/providerFactory"; | ||
| import { AWSSecretsManagerProvider } from "./providerService/awsSecretManagerProvider"; | ||
| import { HashicorpVaultProvider } from "./providerService/hashicorpVaultProvider"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for createTypedProviderInstance usage and definition
rg -n "createTypedProviderInstance" srcRepository: requestly/requestly-desktop-app
Length of output: 449
🏁 Script executed:
# Check what is exported from providerFactory.ts
cat src/lib/secretsManager/providerService/providerFactory.tsRepository: requestly/requestly-desktop-app
Length of output: 1331
🏁 Script executed:
# Check TypeScript configuration
find . -name "tsconfig*.json" -type f -exec echo "=== {} ===" \; -exec cat {} \;Repository: requestly/requestly-desktop-app
Length of output: 867
Missing export for createTypedProviderInstance causes TypeScript compilation failure.
Line 15 imports createTypedProviderInstance from providerFactory, but the module only exports createProviderInstance. Since this file is included in TypeScript compilation, the build will fail with an unresolved import error. Add the export to providerFactory.ts:
Fix: Add typed helper in providerFactory.ts
export function createTypedProviderInstance<T extends SecretProviderType>(
config: ProviderConfigForProvider<T>
): AbstractSecretProvider<T> {
return createProviderInstance(config) as AbstractSecretProvider<T>;
}🤖 Prompt for AI Agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts` around lines 8 - 17, The import of
createTypedProviderInstance in USAGE_EXAMPLES.ts fails because providerFactory
does not export it; add a typed helper export in providerFactory that wraps
createProviderInstance: declare and export createTypedProviderInstance<T extends
SecretProviderType>(config: ProviderConfigForProvider<T>) returning
AbstractSecretProvider<T> and implement it by delegating to
createProviderInstance(config) and casting the result to
AbstractSecretProvider<T>; this ensures the named export exists and preserves
the typed API used by USAGE_EXAMPLES.ts.
| async function example5() { | ||
| const awsProvider = new AWSSecretsManagerProvider(awsConfig); | ||
| const vaultProvider = new HashicorpVaultProvider(vaultConfig); | ||
|
|
||
| const awsRef: AwsSecretReference = { | ||
| type: SecretProviderType.AWS_SECRETS_MANAGER, | ||
| identifier: "my-secret", | ||
| }; | ||
|
|
||
| const vaultRef: VaultSecretReference = { | ||
| type: SecretProviderType.HASHICORP_VAULT, | ||
| path: "secret/data/myapp/config", | ||
| }; | ||
|
|
||
| // Set a single secret - both string and object values are supported | ||
| await awsProvider.setSecret(awsRef, "my-secret-value"); | ||
| await vaultProvider.setSecret(vaultRef, { | ||
| database: "postgres://...", | ||
| apiKey: "xyz...", | ||
| }); | ||
|
|
||
| // Batch set | ||
| await awsProvider.setSecrets([ | ||
| { ref: awsRef, value: "value1" }, | ||
| ]); | ||
|
|
||
| await vaultProvider.setSecrets([ | ||
| { | ||
| ref: { type: SecretProviderType.HASHICORP_VAULT, path: "secret/data/app1" }, | ||
| value: { key1: "value1" }, | ||
| }, | ||
| { | ||
| ref: { type: SecretProviderType.HASHICORP_VAULT, path: "secret/data/app2" }, | ||
| value: { key2: "value2" }, | ||
| }, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Example uses unimplemented write APIs.
AWSSecretsManagerProvider and HashicorpVaultProvider currently throw "Method not implemented." for setSecret/setSecrets, so this example will throw if executed. Consider marking these calls as not yet supported.
💡 Suggested clarification
- // Set a single secret - both string and object values are supported
+ // NOTE: setSecret/setSecrets are not implemented yet in the providers; these calls will throw until implemented.
+ // Set a single secret - both string and object values are supported📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function example5() { | |
| const awsProvider = new AWSSecretsManagerProvider(awsConfig); | |
| const vaultProvider = new HashicorpVaultProvider(vaultConfig); | |
| const awsRef: AwsSecretReference = { | |
| type: SecretProviderType.AWS_SECRETS_MANAGER, | |
| identifier: "my-secret", | |
| }; | |
| const vaultRef: VaultSecretReference = { | |
| type: SecretProviderType.HASHICORP_VAULT, | |
| path: "secret/data/myapp/config", | |
| }; | |
| // Set a single secret - both string and object values are supported | |
| await awsProvider.setSecret(awsRef, "my-secret-value"); | |
| await vaultProvider.setSecret(vaultRef, { | |
| database: "postgres://...", | |
| apiKey: "xyz...", | |
| }); | |
| // Batch set | |
| await awsProvider.setSecrets([ | |
| { ref: awsRef, value: "value1" }, | |
| ]); | |
| await vaultProvider.setSecrets([ | |
| { | |
| ref: { type: SecretProviderType.HASHICORP_VAULT, path: "secret/data/app1" }, | |
| value: { key1: "value1" }, | |
| }, | |
| { | |
| ref: { type: SecretProviderType.HASHICORP_VAULT, path: "secret/data/app2" }, | |
| value: { key2: "value2" }, | |
| }, | |
| ]); | |
| } | |
| async function example5() { | |
| const awsProvider = new AWSSecretsManagerProvider(awsConfig); | |
| const vaultProvider = new HashicorpVaultProvider(vaultConfig); | |
| const awsRef: AwsSecretReference = { | |
| type: SecretProviderType.AWS_SECRETS_MANAGER, | |
| identifier: "my-secret", | |
| }; | |
| const vaultRef: VaultSecretReference = { | |
| type: SecretProviderType.HASHICORP_VAULT, | |
| path: "secret/data/myapp/config", | |
| }; | |
| // NOTE: setSecret/setSecrets are not implemented yet in the providers; these calls will throw until implemented. | |
| // Set a single secret - both string and object values are supported | |
| await awsProvider.setSecret(awsRef, "my-secret-value"); | |
| await vaultProvider.setSecret(vaultRef, { | |
| database: "postgres://...", | |
| apiKey: "xyz...", | |
| }); | |
| // Batch set | |
| await awsProvider.setSecrets([ | |
| { ref: awsRef, value: "value1" }, | |
| ]); | |
| await vaultProvider.setSecrets([ | |
| { | |
| ref: { type: SecretProviderType.HASHICORP_VAULT, path: "secret/data/app1" }, | |
| value: { key1: "value1" }, | |
| }, | |
| { | |
| ref: { type: SecretProviderType.HASHICORP_VAULT, path: "secret/data/app2" }, | |
| value: { key2: "value2" }, | |
| }, | |
| ]); | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts` around lines 181 - 217, The
example5 function uses AWSSecretsManagerProvider.setSecret/setSecrets and
HashicorpVaultProvider.setSecret/setSecrets which currently throw "Method not
implemented."; update the example to avoid invoking unimplemented write APIs by
either (a) replacing those calls with commented lines or a clear "not supported
yet" note, or (b) wrapping each call in a try/catch that logs/ignores the
NotImplemented error, and keep the rest of example5 intact; reference the
AWSSecretsManagerProvider, HashicorpVaultProvider, setSecret, setSecrets, and
example5 symbols when making the change so readers understand these writes are
not yet implemented.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts`:
- Around line 16-18: The code instantiates HashicorpVaultProvider but its import
is commented out, causing a TypeScript compile error; restore or add the correct
import statement for HashicorpVaultProvider (the symbol HashicorpVaultProvider
in USAGE_EXAMPLES.ts) from its providerService module so the class is available
where it’s instantiated (ensure the import path matches
"./providerService/hashicorpVaultProvider" or the actual module export).
- Around line 15-16: The code references createTypedProviderInstance but only
imports createProviderInstance; update the import statement to also import
createTypedProviderInstance from "./providerService/providerFactory" (or the
correct named export from that module) so the usage at lines referencing
createTypedProviderInstance compiles; verify the module actually exports
createTypedProviderInstance and adjust the import name if the export differs,
keeping AWSSecretsManagerProvider import unchanged.
- Around line 63-71: The example2 snippet calls
createProviderInstance(vaultConfig) assuming Vault support, but
providerFactory/createProviderInstance currently only handles AWS, so calling it
will throw; update providerFactory (the factory used by createProviderInstance)
to add a branch that returns a HashiCorp Vault provider when config.type ===
SecretProviderType.HASHICORP_VAULT (using the same AbstractSecretProvider
interface), or alternatively change the USAGE_EXAMPLES.ts example2 to only show
awsProvider and add a note that Vault support is not yet implemented; reference
createProviderInstance, providerFactory, SecretProviderType.HASHICORP_VAULT, and
vaultConfig when making the change.
| import { createProviderInstance } from "./providerService/providerFactory"; | ||
| import { AWSSecretsManagerProvider } from "./providerService/awsSecretManagerProvider"; |
There was a problem hiding this comment.
createTypedProviderInstance is used but not imported (TS compile error).
Line 73 references createTypedProviderInstance, but Line 15 only imports createProviderInstance.
🛠️ Suggested fix
-import { createProviderInstance } from "./providerService/providerFactory";
+import {
+ createProviderInstance,
+ createTypedProviderInstance,
+} from "./providerService/providerFactory";Also applies to: 73-77
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts` around lines 15 - 16, The code
references createTypedProviderInstance but only imports createProviderInstance;
update the import statement to also import createTypedProviderInstance from
"./providerService/providerFactory" (or the correct named export from that
module) so the usage at lines referencing createTypedProviderInstance compiles;
verify the module actually exports createTypedProviderInstance and adjust the
import name if the export differs, keeping AWSSecretsManagerProvider import
unchanged.
| import { AWSSecretsManagerProvider } from "./providerService/awsSecretManagerProvider"; | ||
| // import { HashicorpVaultProvider } from "./providerService/hashicorpVaultProvider"; | ||
|
|
There was a problem hiding this comment.
HashicorpVaultProvider is referenced without an import (TS compile error).
Line 86 instantiates HashicorpVaultProvider while its import is commented out at Line 17.
🛠️ Suggested fix
-// import { HashicorpVaultProvider } from "./providerService/hashicorpVaultProvider";
+import { HashicorpVaultProvider } from "./providerService/hashicorpVaultProvider";Also applies to: 84-87
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts` around lines 16 - 18, The code
instantiates HashicorpVaultProvider but its import is commented out, causing a
TypeScript compile error; restore or add the correct import statement for
HashicorpVaultProvider (the symbol HashicorpVaultProvider in USAGE_EXAMPLES.ts)
from its providerService module so the class is available where it’s
instantiated (ensure the import path matches
"./providerService/hashicorpVaultProvider" or the actual module export).
| async function example2() { | ||
| // Generic factory - returns AbstractSecretProvider<SecretProviderType> | ||
| const awsProvider = createProviderInstance(awsConfig); | ||
| const vaultProvider = createProviderInstance(vaultConfig); | ||
|
|
||
| // TypeScript knows the provider type from the instance | ||
| console.log(awsProvider.type); // SecretProviderType.AWS_SECRETS_MANAGER | ||
| console.log(vaultProvider.type); // SecretProviderType.HASHICORP_VAULT | ||
|
|
There was a problem hiding this comment.
Example 2 assumes factory supports Vault, but providerFactory currently only handles AWS.
createProviderInstance(vaultConfig) will throw unless the factory is extended.
Consider either extending the factory to support SecretProviderType.HASHICORP_VAULT or annotating this example as not yet supported to avoid runtime errors.
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/USAGE_EXAMPLES.ts` around lines 63 - 71, The example2
snippet calls createProviderInstance(vaultConfig) assuming Vault support, but
providerFactory/createProviderInstance currently only handles AWS, so calling it
will throw; update providerFactory (the factory used by createProviderInstance)
to add a branch that returns a HashiCorp Vault provider when config.type ===
SecretProviderType.HASHICORP_VAULT (using the same AbstractSecretProvider
interface), or alternatively change the USAGE_EXAMPLES.ts example2 to only show
awsProvider and add a note that Vault support is not yet implemented; reference
createProviderInstance, providerFactory, SecretProviderType.HASHICORP_VAULT, and
vaultConfig when making the change.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.