Feature/v0.4.0#73
Merged
Merged
Conversation
WalkthroughThis PR centralizes persisted CLI state in a new versioned secureStore (auth + secret-cache) with legacy migration, adds git-aware configuration scoping (path vs git) and surfaces scope to the configure command and client, and implements an ek scan command using Betterleaks with binary install, JSON parsing, Ink-based report UI, and analytics integration. Tests were added/updated to cover secureStore, config scoping, scan behavior, and migration paths. The package version is bumped to 0.4.0. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 6
🧹 Nitpick comments (3)
src/lib/secureStore.ts (1)
49-50: ⚡ Quick winUnsafe type cast after partial validation.
After checking
parsed.version === SECURE_STORE_VERSION, the code casts toSecureStoreDatawithout validatingauthorsecretCacheshapes. If stored data has a correct version but malformed nested fields, downstream code could fail unexpectedly.Consider validating nested fields or treating invalid shapes as corrupted data (returning empty store).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/secureStore.ts` around lines 49 - 50, The current branch returns parsed as SecureStoreData after only checking isRecord(parsed) and parsed.version === SECURE_STORE_VERSION, which is unsafe because nested fields like auth and secretCache aren't validated; update the logic in src/lib/secureStore.ts so that after the version check you explicitly validate the shapes of parsed.auth and parsed.secretCache (e.g., add helper predicates or inline checks similar to isRecord for those nested objects) and only return { store: parsed as SecureStoreData, legacyAuth: false } when those validations pass; if nested fields are missing or malformed, treat the data as corrupted and return an empty store (or the existing empty-store fallback) instead of casting blindly.src/lib/secretCache.ts (1)
52-54: 💤 Low valueOutdated comment references keyring.
The comment says "Keyring unavailable" but storage now uses
secureStore. Update for clarity.📝 Proposed fix
} catch { - // Keyring unavailable; silently degrade + // Secure store unavailable; silently degrade }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/secretCache.ts` around lines 52 - 54, Update the misleading catch comment in src/lib/secretCache.ts to reflect the current storage mechanism: replace "Keyring unavailable; silently degrade" with a message mentioning secureStore (e.g., "secureStore unavailable; silently degrade") in the catch block where secret retrieval/fallback occurs (the catch inside the secret cache/secret retrieval logic that references secureStore). Ensure the comment accurately references the symbol secureStore so future readers understand which storage is degrading.src/lib/sharedHttpClient.ts (1)
6-11: 💤 Low valueRemove unused
keyringKeyfield fromHttpClientConfig.The
keyringKeyfield inHttpClientConfigis no longer referenced in the implementation since token retrieval now usessecureStore.getAuth(). Remove it from the type definition and all call sites to eliminate dead code and reduce confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/sharedHttpClient.ts` around lines 6 - 11, Remove the now-unused keyringKey property from the HttpClientConfig type and from any places that construct or reference that config; update the type declaration for HttpClientConfig (remove keyringKey) and modify callers that pass or read keyringKey to instead rely on secureStore.getAuth() / existing token retrieval logic (e.g., where HttpClientConfig is constructed or consumed in functions/classes using sharedHttpClient). Ensure no leftover references to keyringKey remain in types, constructors, or call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.ts`:
- Around line 46-47: The current check uses process.argv[2] === "scan" which
fails if flags precede the subcommand; update the logic used when calling
analytics.init({ skipAuthLookup: ... }) to locate the first non-flag CLI
argument (e.g., find the first element in process.argv.slice(2) that does not
start with '-') and compare that value to "scan". Change the condition used in
analytics.init to use this firstNonFlagArg check so skipAuthLookup is true when
the detected subcommand is "scan".
In `@src/cmd/configure.ts`:
- Around line 27-33: The current logic defaults to "git" when selectName(...)
returns no value; update the function that calls selectName (referencing
selectName, GIT_SCOPE_LABEL and PATH_SCOPE_LABEL) to explicitly handle a
canceled/undefined selection by aborting: if selectedScope is
null/undefined/empty, do not return "git"—instead throw or return a clear
cancellation signal (e.g., throw a user-cancelled Error or return
undefined/null) so callers know the user aborted and no config is saved under an
unintended scope.
In `@src/cmd/sdk.ts`:
- Around line 33-37: The current empty catch around
config.findProjectConfig(process.cwd()) swallows all errors; change it to catch
the error as a variable (e) and only suppress the expected "no project
configured" condition (check error type/message or a specific error class
returned by config.findProjectConfig), but rethrow or surface any other errors
so real failures aren't masked; update the try/catch around setup = await
config.findProjectConfig(process.cwd()) to implement this selective handling.
In `@src/lib/betterleaks.ts`:
- Around line 76-83: The code downloads a remote archive (downloadUrl) to
archivePath using axios.get and then extracts it via execFileSync based on
assetName without verifying integrity; update the flow in the function that
performs the download so you compute a cryptographic hash (e.g., SHA-256) of
response.data after axios.get and before fs.writeFileSync, compare it against a
trusted expected checksum (fetched from release metadata, a .sha256 file, or
provided configuration), and only write and extract when the hashes match;
alternatively, if a signature (.asc/.sig) is available, verify the detached GPG
signature before extraction; on mismatch or failed signature verification,
throw/exit and avoid calling execFileSync (keep variables downloadUrl,
response.data, archivePath, assetName, tmpDir and the extraction logic intact
but gated by the verification step).
- Around line 148-164: The current code awaits proc.exited (the Bun.spawn child)
with no timeout; implement a timeout guard around awaiting proc.exited (use
Promise.race or equivalent) so that if the Betterleaks process hangs past a
configured timeout you call proc.kill() to terminate it and return/map the
failure to SCAN_RUN_FAILED; update the logic around the Bun.spawn call
(referencing proc, Bun.spawn, proc.exited and reportPath) to clear any timers,
handle the killed/timeout branch deterministically, and ensure the function
returns the SCAN_RUN_FAILED result on timeout.
In `@src/lib/secureStore.ts`:
- Around line 75-79: updateStore currently does a non-atomic read-modify-write
(readStore -> updater -> writeStore) and can lose concurrent updates (e.g.,
concurrent calls from setAuth or setSecretCacheEntry); wrap the
read/update/write with a local async mutex to serialize access: acquire the
mutex at the start of updateStore, call readStore, run the updater, call
writeStore, and always release the mutex in a finally block to avoid deadlocks;
keep the mutex file-local (e.g., a Promise-based lock or a small Lock class) so
callers (setAuth, setSecretCacheEntry) need no changes and all updates become
atomic.
---
Nitpick comments:
In `@src/lib/secretCache.ts`:
- Around line 52-54: Update the misleading catch comment in
src/lib/secretCache.ts to reflect the current storage mechanism: replace
"Keyring unavailable; silently degrade" with a message mentioning secureStore
(e.g., "secureStore unavailable; silently degrade") in the catch block where
secret retrieval/fallback occurs (the catch inside the secret cache/secret
retrieval logic that references secureStore). Ensure the comment accurately
references the symbol secureStore so future readers understand which storage is
degrading.
In `@src/lib/secureStore.ts`:
- Around line 49-50: The current branch returns parsed as SecureStoreData after
only checking isRecord(parsed) and parsed.version === SECURE_STORE_VERSION,
which is unsafe because nested fields like auth and secretCache aren't
validated; update the logic in src/lib/secureStore.ts so that after the version
check you explicitly validate the shapes of parsed.auth and parsed.secretCache
(e.g., add helper predicates or inline checks similar to isRecord for those
nested objects) and only return { store: parsed as SecureStoreData, legacyAuth:
false } when those validations pass; if nested fields are missing or malformed,
treat the data as corrupted and return an empty store (or the existing
empty-store fallback) instead of casting blindly.
In `@src/lib/sharedHttpClient.ts`:
- Around line 6-11: Remove the now-unused keyringKey property from the
HttpClientConfig type and from any places that construct or reference that
config; update the type declaration for HttpClientConfig (remove keyringKey) and
modify callers that pass or read keyringKey to instead rely on
secureStore.getAuth() / existing token retrieval logic (e.g., where
HttpClientConfig is constructed or consumed in functions/classes using
sharedHttpClient). Ensure no leftover references to keyringKey remain in types,
constructors, or call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ceec6b2-6258-43a5-9128-13d6f098737c
📒 Files selected for processing (29)
package.jsonsrc/api/auth.tssrc/api/client.tssrc/cli.tssrc/cmd/configure.tssrc/cmd/index.tssrc/cmd/login.tssrc/cmd/logout.tssrc/cmd/scan.tssrc/cmd/sdk.tssrc/cmd/whoami.tssrc/lib/analytics.tssrc/lib/betterleaks.tssrc/lib/config.tssrc/lib/errors.tssrc/lib/git.tssrc/lib/secretCache.tssrc/lib/secureStore.tssrc/lib/sharedHttpClient.tssrc/ui/ScanReport.tsxsrc/ui/Spinner.tsxtests/integration/config.test.tstests/integration/configure-command.test.tstests/integration/configure-flow.test.tstests/integration/http-client.test.tstests/integration/login-flow.test.tstests/integration/logout-command.test.tstests/integration/secret-cache.test.tstests/integration/secure-store.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/analytics.ts`:
- Around line 136-148: The "version_usage" analytics event is emitted every time
init() runs, causing duplicate events in the same process; add a module-level
boolean flag (e.g., sentVersionUsage or versionUsageEmitted) and check it inside
init() before calling analytics.track("version_usage", ...), only calling
analytics.track when the flag is false, then set the flag to true immediately
after sending to ensure the event is emitted once per process; reference init(),
analytics.track("version_usage", ...) and the new
sentVersionUsage/versionUsageEmitted symbol in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e9d71e9-a6f4-4632-8089-5d92db5901ce
📒 Files selected for processing (1)
src/lib/analytics.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/secureStore.ts (1)
102-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute every
SECURE_STORE_KEYmutation through this mutex.
updateStore()is serialized now, butreadStore({ upgradeLegacy: true })still callswriteStore()outside the lock andclearAll()deletes the same key without it. A concurrent legacy upgrade or clear can still land aftersetAuth()/setSecretCacheEntry()and drop the newer state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/secureStore.ts` around lines 102 - 109, The SECURE_STORE_KEY mutations must be serialized: ensure any code path that can write or delete that key acquires storeMutex before reading/upgrading/writing. Specifically, change readStore({upgradeLegacy: true}) so the legacy-upgrade path performs its writeStore() while holding storeMutex (or call updateStore under the lock), and make clearAll() acquire/release storeMutex around the deletion of SECURE_STORE_KEY; also ensure setAuth() and setSecretCacheEntry() call writeStore() only while holding storeMutex (or route them through updateStore()). In short, wrap readStore(upgradeLegacy:true) upgrade writes, clearAll key deletion, and all set*/writeStore calls with storeMutex.acquire()/release() or centralize mutation logic behind updateStore() so every mutation goes through the same mutex.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/betterleaks.ts`:
- Around line 185-193: The race currently awaits proc.exited but ignores its
exit code; modify the Promise.race to capture the resolved exit code (e.g.,
const exitCode = await Promise.race([proc.exited, new Promise<never>((_, reject)
=> { timeoutId = setTimeout(() => reject(new Error("timeout")), SCAN_TIMEOUT);
})])); then check exitCode !== 0 and if so throw the existing SCAN_RUN_FAILED
error (matching the pattern used in sdk.ts and run.ts); also ensure you
clearTimeout(timeoutId) on success/failure and propagate the timeout error as
before.
---
Duplicate comments:
In `@src/lib/secureStore.ts`:
- Around line 102-109: The SECURE_STORE_KEY mutations must be serialized: ensure
any code path that can write or delete that key acquires storeMutex before
reading/upgrading/writing. Specifically, change readStore({upgradeLegacy: true})
so the legacy-upgrade path performs its writeStore() while holding storeMutex
(or call updateStore under the lock), and make clearAll() acquire/release
storeMutex around the deletion of SECURE_STORE_KEY; also ensure setAuth() and
setSecretCacheEntry() call writeStore() only while holding storeMutex (or route
them through updateStore()). In short, wrap readStore(upgradeLegacy:true)
upgrade writes, clearAll key deletion, and all set*/writeStore calls with
storeMutex.acquire()/release() or centralize mutation logic behind updateStore()
so every mutation goes through the same mutex.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c992e08-8d81-4107-b349-db5b9cf998db
📒 Files selected for processing (2)
src/lib/betterleaks.tssrc/lib/secureStore.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/betterleaks.ts (1)
100-104: 💤 Low valueUse Node.js zip library or PowerShell's
Expand-Archiveinstead oftarfor Windows.zipextraction.Windows downloads
.zipfiles (viagetAssetName()line 54), but the extraction at lines 101-102 usestar -xf, which is unreliable on Windows. Older Windows versions lacktar, and even Windows 10+ may not handle.zipconsistently. Consider usingadm-ziporunzipper(or PowerShell'sExpand-Archiveas a fallback) for cross-platform compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/betterleaks.ts` around lines 100 - 104, The .zip extraction currently calls execFileSync("tar", ...) which fails on many Windows systems; update the extraction branch in src/lib/betterleaks.ts (the block that checks assetName.endsWith(".zip") and uses execFileSync with archivePath and tmpDir) to use a cross-platform ZIP extractor instead: either integrate a Node ZIP library (e.g., adm-zip or unzipper) to programmatically extract archivePath into tmpDir, or detect Windows (process.platform === "win32") and call PowerShell's Expand-Archive via spawnSync as a fallback, preserving error handling and the same tmpDir target and stdio behavior as the non-zip branch; keep the non-zip tar extraction path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/betterleaks.ts`:
- Around line 100-104: The .zip extraction currently calls execFileSync("tar",
...) which fails on many Windows systems; update the extraction branch in
src/lib/betterleaks.ts (the block that checks assetName.endsWith(".zip") and
uses execFileSync with archivePath and tmpDir) to use a cross-platform ZIP
extractor instead: either integrate a Node ZIP library (e.g., adm-zip or
unzipper) to programmatically extract archivePath into tmpDir, or detect Windows
(process.platform === "win32") and call PowerShell's Expand-Archive via
spawnSync as a fallback, preserving error handling and the same tmpDir target
and stdio behavior as the non-zip branch; keep the non-zip tar extraction path
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf8ddb59-9816-4f33-b6d3-116d542bb6f5
📒 Files selected for processing (1)
src/lib/betterleaks.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Version