[DB-21] added variable fetching flow and awsSecretsManagerProvider #266
Conversation
WalkthroughThis PR implements AWS Secrets Manager integration into the application. It adds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Key Changespackage.json
AbstractSecretProvider.ts
AWSSecretsManagerProvider.ts
SecretsManager.ts
types.ts
events.js
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/secretsManager/secretsManager.ts (1)
15-18: Do not log provider configs (credentials leak).Line 16 logs the full provider config, which can include access key material. Remove or redact this log.
Proposed fix
async addProviderConfig(config: SecretProviderConfig) { - console.log("!!!debug", "addconfig", config); await this.registry.setProviderConfig(config); }
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/providerService/AbstractSecretProvider.ts`:
- Around line 35-40: The validateConfig method in AbstractSecretProvider
currently always returns false, causing all configs to be rejected; update
AbstractSecretProvider.validateConfig to perform a sensible default check (e.g.,
ensure config is non-null and of expected type/shape) or, as a permissive
placeholder until provider-specific checks are added, return true when config
exists; locate the static validateConfig method on the AbstractSecretProvider
class and replace the unconditional "return false" with either a real validation
routine (checking required keys/types) or a permissive "return true" when config
is provided.
In `@src/lib/secretsManager/providerService/awsSecretManagerProvider.ts`:
- Around line 74-120: The getSecret method logs full secret objects
(cachedSecret and awsSecret) and raw secretResponse, which can expose
SecretString; remove or sanitize these logs in
AwsSecretManagerProvider.getSecret so no SecretString or AwsSecretValue.value is
ever printed. Replace console.log/console.error calls to only emit non-sensitive
metadata (e.g., ref.nameOrArn, response.$metadata.httpStatusCode,
response.VersionId, response.ARN) or use a redacted placeholder like
"[REDACTED]" for any value fields, and ensure the cache key and cachedSecret
logging do not include the secret value; update calls referencing cachedSecret
and awsSecret accordingly.
- Around line 122-202: getSecrets currently batches all refs into one
BatchGetSecretValueCommand which ignores per-ref VersionId, can exceed AWS's
20-secret limit, and drops per-secret errors; fix by splitting refs into two
flows: for refs with a VersionId call the existing getSecret(ref) (or a new
helper that uses GetSecretValue for a single SecretId+VersionId) so the returned
value matches the versioned cache key (use getCacheKey/ref when caching), and
for unversioned refs chunk them into groups of <=20 before constructing
BatchGetSecretValueCommand; after each client.send(batchCmd) inspect
response.Errors and map each Error.SecretId to the corresponding ref (log/skip
failed secrets) and only cache/append successful entries from
response.SecretValues, preserving existing checks (Name/SecretString) and TTL
logic (DEFAULT_CACHE_TTL_MS) and keep using this.cache and this.id for
providerId.
In `@src/lib/secretsManager/secretsManager.ts`:
- Around line 68-75: In fetchSecrets, the provider existence check is inverted:
registry.getProvider(providerId) is truthy but the code logs "not found" and
proceeds to call provider.getSecrets; flip the condition to log a warning when
provider is missing and continue; specifically, in the loop over providerMap in
fetchSecrets, after calling this.registry.getProvider(providerId) ensure you if
(!provider) { console.warn(`Provider with id ${providerId} not found`);
continue; } before calling provider.getSecrets(refs) and pushing results.
In `@src/main/events.js`:
- Around line 297-339: Handlers registered via ipcMain.handle call methods on
the module-scoped secretsManager (e.g., secretsManager.addProviderConfig,
secretsManager.getProviderConfig, secretsManager.removeProviderConfig,
secretsManager.testProviderConnection, secretsManager.fetchSecret) but
secretsManager can be null until init-secretsManager runs; add a null-check at
the start of each handler (or a small helper that ensures/lazily initializes
secretsManager) and return/throw a clear error result if secretsManager is not
initialized so the renderer receives a predictable failure instead of a runtime
"Cannot read properties of null" exception.
- Around line 304-337: Remove the debug console logging that exposes credentials
in the IPC handlers: eliminate or redact the console.log calls inside the
"secretsManager:getProviderConfig" and "secretsManager:resolveSecret" handlers
so providerConfig and secretValue are not logged; locate the ipcMain.handle
callbacks that call secretsManager.getProviderConfig(id) and
secretsManager.fetchSecret(providerId, ref) (and any other secretsManager.*
calls in these handlers) and either delete the console.log statements or replace
them with non-sensitive logs (e.g., log only operation success/failure or masked
IDs) to avoid printing secrets to stdout.
🧹 Nitpick comments (2)
src/lib/secretsManager/types.ts (1)
1-46: Consider supportingSecretBinaryinAwsSecretValueif binary secrets are needed.
GetSecretValueCommandOutputcan returnSecretBinarywithoutSecretString, but the current provider implementation rejects such secrets (lines 100, 162 inawsSecretManagerProvider.ts). If binary secret support is required, either addSecretBinaryto the type or explicitly document that only string secrets are supported.Suggested type expansion
export interface AwsSecretValue extends BaseSecretValue { name: GetSecretValueCommandOutput["Name"]; value: GetSecretValueCommandOutput["SecretString"]; + binary?: GetSecretValueCommandOutput["SecretBinary"]; ARN: GetSecretValueCommandOutput["ARN"]; versionId: GetSecretValueCommandOutput["VersionId"]; }src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
204-218: Prefer a typed “unsupported” error for mutation APIs.
A custom error (or error code) makes it easier for callers to branch on read-only providers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc/lib/secretsManager/providerService/AbstractSecretProvider.tssrc/lib/secretsManager/providerService/awsSecretManagerProvider.tssrc/lib/secretsManager/secretsManager.tssrc/lib/secretsManager/types.tssrc/main/events.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib/secretsManager/secretsManager.ts (1)
src/lib/secretsManager/types.ts (2)
SecretReference(31-31)SecretValue(46-46)
src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
src/lib/secretsManager/types.ts (4)
AWSSecretsManagerConfig(7-12)AwsSecretValue(39-44)SecretProviderConfig(16-23)AwsSecretReference(25-29)
src/lib/secretsManager/providerService/AbstractSecretProvider.ts (1)
src/lib/secretsManager/types.ts (2)
SecretValue(46-46)SecretReference(31-31)
🔇 Additional comments (7)
package.json (2)
257-257: This concern is not applicable—source-map-supportis only imported in development mode.The import at
src/main/main.ts:72is guarded by anif (isDevelopment)block that only executes whenNODE_ENV === "development"orDEBUG_PROD === "true". In production packaged builds, this code path never runs, so movingsource-map-supporttodevDependenciesis correct and safe.Likely an incorrect or invalid review comment.
270-270: AWS SDK dependency properly isolated to main process.Verification confirms
@aws-sdk/client-secrets-manageris imported only insrc/main/events.jsand used viasrc/lib/secretsManager/library code. The renderer code has no imports of this dependency, ensuring it stays out of renderer bundles as intended.src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (5)
31-43: Constructor setup looks solid.
Clean initialization of the Secrets Manager client with the provided config.
45-47: Cache key composition is clear.
49-72: Connection test flow is reasonable.
220-228: Refresh flow is clean.
230-234: Config validation is concise.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/lib/secretsManager/providerService/awsSecretManagerProvider.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts`:
- Around line 86-90: The current try/catch in encryptedFsStorage.ts that does
JSON.parse(decryptedString) and on error returns decryptedString as unknown as T
must be changed to avoid silently returning a wrong type; either throw a new
Error (wrap the original parse error with context including the decryptedString
or the storage key) or change the function signature to return a discriminated
union like { success: true, value: T } | { success: false, raw: string, error:
Error } so callers can handle failures explicitly; update the code surrounding
JSON.parse(decryptedString) (reference decryptedString and the JSON.parse call
in encryptedFsStorage.ts) and adjust callers to handle the thrown error or the
new discriminated result accordingly.
In `@src/lib/secretsManager/secretsManager.ts`:
- Around line 58-60: The current setProviderConfig method contains a debug
console.log that prints the full provider config (sensitive), so remove that
console.log("!!!debug", "addconfig", config) and either (a) delete it entirely,
or (b) replace it with a safe, minimal log that does not include secrets (e.g.,
log only config.id or provider type) or a masked version of sensitive fields, or
(c) gate detailed logging behind a strict secure debug flag checked before
logging; update setProviderConfig and any callers accordingly and ensure
this.registry.setProviderConfig(config) remains unchanged.
- Around line 135-146: The example docs use the old method name fetchSecret;
update the example to call the current API getSecret instead: change the usage
in the comment block that shows SecretsManager.initialize(registry) and
getSecretsManager() to call await getSecretsManager().getSecret(providerId, ref)
so the example matches the SecretsManager API.
- Around line 20-34: The initialize method can leave a partially-initialized
singleton when registry.initialize() throws; modify SecretsManager.initialize so
it does not keep this.instance or this.initPromise set on failure — either
create the SecretsManager instance only after await registry.initialize()
succeeds, or wrap the current initPromise body in a try/catch that on any error
resets this.instance = undefined and this.initPromise = undefined before
rethrowing; ensure getInstance() will not return a bad instance and callers can
retry initialization.
♻️ Duplicate comments (4)
src/main/events.js (2)
293-335: Guard againstsecretsManagerbeing uninitialized.These handlers access
secretsManager.*directly, butsecretsManagerisnulluntilinit-secretsManagerruns. Any call before initialization will throwCannot read properties of null.Add a guard at the start of each handler or use a helper function:
Proposed helper approach
+ const ensureSecretsManager = () => { + if (!secretsManager) { + throw new Error("SecretsManager not initialized. Call init-secretsManager first."); + } + return secretsManager; + }; + ipcMain.handle( "secretsManager:addProviderConfig", async (event, { config }) => { - await secretsManager.setProviderConfig(config); + await ensureSecretsManager().setProviderConfig(config); } );
302-302: Debug logs exposing sensitive data still present.Lines 302 and 327-332 log provider configs and secret values. As previously noted, these should be removed before merging.
Also applies to: 327-332
src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
56-56: Debug logs exposing secret values still present.Multiple
console.logandconsole.errorstatements print full secret objects includingSecretString. As previously acknowledged, these should be removed before merging.Also applies to: 83-83, 95-95, 100-100, 114-114
src/lib/secretsManager/secretsManager.ts (1)
111-119: Handle missing providers explicitly (duplicate).This is the same issue noted previously: missing providers are silently skipped. Please warn/continue (or throw) when
provideris undefined.✅ Reminder fix
for (const [providerId, refs] of providerMap.entries()) { const provider = this.registry.getProvider(providerId); - if (provider) { - const secretValues = await provider.getSecrets(refs); - results.push( - ...secretValues.filter((sv): sv is SecretValue => sv !== null) - ); - } + if (!provider) { + console.warn(`Provider with id ${providerId} not found`); + continue; + } + const secretValues = await provider.getSecrets(refs); + results.push( + ...secretValues.filter((sv): sv is SecretValue => sv !== null) + ); }
🧹 Nitpick comments (1)
src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
44-46: Minor: Inconsistent delimiter in cache key.The cache key uses mixed delimiters:
name=with;separator butversion:with colon. Consider using consistent formatting.Suggested fix
protected getCacheKey(ref: AwsSecretReference): string { - return `name=${ref.identifier};version:${ref.version}`; + return `name=${ref.identifier};version=${ref.version ?? ""}`; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.tssrc/lib/secretsManager/providerService/AbstractSecretProvider.tssrc/lib/secretsManager/providerService/awsSecretManagerProvider.tssrc/lib/secretsManager/secretsManager.tssrc/lib/secretsManager/types.tssrc/main/events.js
🧰 Additional context used
🧬 Code graph analysis (5)
src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
src/lib/secretsManager/types.ts (4)
AWSSecretsManagerConfig(7-12)AwsSecretValue(39-44)SecretProviderConfig(16-23)AwsSecretReference(25-29)
src/lib/secretsManager/secretsManager.ts (1)
src/lib/secretsManager/types.ts (3)
SecretProviderConfig(16-23)SecretReference(31-31)SecretValue(46-46)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (1)
src/renderer/actions/local-sync/fs-utils.ts (2)
writeContentRaw(315-351)deleteFsResource(98-142)
src/main/events.js (1)
src/lib/secretsManager/secretsManager.ts (2)
SecretsManager(5-133)getSecretsManager(147-149)
src/lib/secretsManager/providerService/AbstractSecretProvider.ts (1)
src/lib/secretsManager/types.ts (2)
SecretValue(46-46)SecretReference(31-31)
🔇 Additional comments (6)
src/lib/secretsManager/encryptedStorage/encryptedFsStorage.ts (2)
59-65: LGTM!The explicit result-type check and error propagation for
writeContentRawaligns well with the result-based API pattern used infs-utils.ts.
100-103: LGTM!Consistent with the
savemethod—explicit error propagation for delete operations.src/lib/secretsManager/types.ts (1)
33-46: LGTM!The type hierarchy is well-structured. Using indexed access types from
GetSecretValueCommandOutputensures theAwsSecretValuefields stay in sync with the AWS SDK. TheBaseSecretValueprovides a clean foundation for future provider implementations.src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (2)
121-130: LGTM!Good decision to avoid
BatchGetSecretValueCommand—it requires additional IAM permissions (secretsmanager:BatchGetSecretValue) and doesn't support per-secretVersionId. The comment clearly documents the reasoning.
148-156: Consider edge case: secrets removed externally during refresh.
refreshSecretscollects refs from cached values, clears the cache, then re-fetches. If a secret was deleted from AWS between caching and refresh,getSecretwill returnnullbut the operation continues gracefully. This is acceptable behavior, but consider logging which secrets failed to refresh for observability.src/lib/secretsManager/providerService/AbstractSecretProvider.ts (1)
1-43: LGTM!The abstract class provides a clean contract for secret providers:
- Updated types align with the new
SecretValuedefinitionsinvalidateCache()as a protected method allows subclasses to clear state during refresh- The fail-safe
validateConfigpattern (always returnsfalsein base) ensures providers must implement validation
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/providerService/AbstractSecretProvider.ts`:
- Around line 55-63: The loop in setCacheEntry can become infinite when
maxCacheSize <= 0; modify setCacheEntry (and/or enforce in the class) to guard
against non-positive values by returning early if this.maxCacheSize <= 0 or by
clamping it to at least 1 before the while loop; ensure you still call
evictExpiredEntries and reference this.cache, this.maxCacheSize, and the
existing logic that deletes oldestKey so the rest of the method behaves
correctly for normal positive sizes.
In `@src/lib/secretsManager/secretsManager.ts`:
- Around line 144-151: Update the doc example to call getSecret with the
required parameters: include the providerId and ref when invoking getSecret on
the SecretsManager instance shown in the snippet; keep the existing
initialization flow (SecretsManager.initialize(registry)) and
import/getSecretsManager usage, but change the example call to await
secretsManager.getSecret(providerId, ref) (using the same variable names to
match the getSecret signature).
♻️ Duplicate comments (3)
src/lib/secretsManager/secretsManager.ts (2)
117-122: Consider warning when a provider is missing.
Silently skipping unknown provider IDs can hide misconfiguration; a warning improves debuggability.🔎 Suggested tweak
const provider = this.registry.getProvider(providerId); if (!provider) { + console.warn(`Provider with id ${providerId} not found`); continue; }
64-66: Remove provider config logging to avoid credential leakage.
This logs full provider config (including secrets). Please drop it or redact.🧹 Proposed fix
async setProviderConfig(config: SecretProviderConfig) { - console.log("!!!debug", "addconfig", config); await this.registry.setProviderConfig(config); }src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (1)
77-110: Remove/sanitize secret-bearing logs.
cachedSecret,awsSecret, andsecretResponsecan includeSecretStringand other sensitive data. Logging these risks credential leakage. Please remove or redact to metadata only.🔒 Proposed sanitization
- if (cachedSecret) { - console.log("!!!debug", "returning from cache", cachedSecret); - return cachedSecret; - } + if (cachedSecret) { + console.log("!!!debug", "returning from cache", { + name: cachedSecret.name, + ARN: cachedSecret.ARN, + versionId: cachedSecret.versionId, + }); + return cachedSecret; + } ... - if (secretResponse.$metadata.httpStatusCode !== 200) { - console.error("!!!debug", "Failed to fetch secret", secretResponse); + if (secretResponse.$metadata.httpStatusCode !== 200) { + console.error("!!!debug", "Failed to fetch secret", { + secretId: ref.identifier, + httpStatusCode: secretResponse.$metadata.httpStatusCode, + }); return null; } ... - if (!secretResponse.SecretString) { - console.error("!!!debug", "SecretString is empty", secretResponse); + if (!secretResponse.SecretString) { + console.error("!!!debug", "SecretString is empty", { + secretId: ref.identifier, + httpStatusCode: secretResponse.$metadata.httpStatusCode, + }); return null; } ... - console.log("!!!debug", "returning after fetching", awsSecret); + console.log("!!!debug", "returning after fetching", { + name: awsSecret.name, + ARN: awsSecret.ARN, + versionId: awsSecret.versionId, + });
🧹 Nitpick comments (2)
src/lib/secretsManager/providerService/awsSecretManagerProvider.ts (2)
44-66:testConnectionmay fail under least‑privilege IAM policies.
ListSecretsrequires broader permissions thanGetSecretValue. If the intended policy is least‑privilege (secret‑specific), consider a lighter probe (e.g., a configured test secret or identity call), or treatAccessDeniedon list as “credentials ok but missing list permission.”
127-141: No production call sites found—methods are safely unreachable.
Verification confirms zero callers forsetSecret(),setSecrets(),removeSecret(), andremoveSecrets(). If these methods are intended for future use, consider a more descriptive error message (e.g., "AWS Secrets Manager does not support write/delete operations") instead of the generic "Method not implemented."
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.