1password/integration #15
Conversation
Ddded option to use CLI in server environments
bump version
UI is still a work in progress and needs further refinement
…ist commands and more Start simplifying the UI to make it clearer and easier to use Remove unnecessary duplicate code and fix a few minor bugs
i still have to do the UI for the table and fixing bug with the delete secret.
…x some bugs and cleaning some code.
…ble components, improved error handling, implemented feedback from Notion, fixed reported bugs, and added a README explaining the project and contribution guidelines.
… for windows mac and linux. tested on mac need to test the other os
… UI, and perform testing
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughComplete migration from Go CLI to TypeScript/Bun-based CLI. Removes Go implementation (cmd/, internal/, main.go, go.mod) and adds TypeScript/JavaScript equivalents: CLI entrypoint, commands, multiple provider integrations (Enkryptify, AWS, GCP, OnePassword), interactive UI layer using Ink, library utilities for config/auth/keyring management, and supporting tooling configurations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Areas requiring focused attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
README.md-76-76 (1)
76-76: Fix typo in directory path.The directory name "enkryptfiy" should be "enkryptify".
-│ └── enkryptfiy/ # Enkryptify provider +│ └── enkryptify/ # Enkryptify providersrc/lib/keyring.ts-35-38 (1)
35-38:has()cannot distinguish between "key not found" and "keyring error".Since
get()returnsnullon both missing keys and errors,has()will returnfalsein both cases. This could mask keyring access failures, potentially causing the application to behave as if credentials don't exist when they actually do..github/workflows/release.yml-166-168 (1)
166-168: Updatesoftprops/action-gh-releaseto v2.The action version
v1is outdated and may have compatibility issues with newer GitHub Actions runners. Update tov2for the latest features and fixes.- name: Create GitHub Release if: github.ref_type == 'tag' - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2.github/workflows/release.yml-133-138 (1)
133-138: Use safer glob pattern to avoid argument parsing issues.The
sha256sum *pattern could misinterpret filenames starting with-as options (SC2035). Use./*or add--to prevent this.run: | set -euo pipefail cd dist/archives - sha256sum * > checksums.txt + sha256sum ./* > checksums.txtsrc/complete/ek.bash-2-4 (1)
2-4: Remove unused variable.The
curvariable is declared and assigned but never used. The completion logic populatesCOMPREPLYdirectly viamapfile, makingcurunnecessary.Apply this diff:
_ek() { - local cur COMPREPLY=() - cur="${COMP_WORDS[COMP_CWORD]}" mapfile -t COMPREPLY < <(ek __complete "${COMP_WORDS[@]:1}") }src/complete/ek.ps1-10-13 (1)
10-13: Avoid using$argsas it's a reserved automatic variable in PowerShell.
$argsis a built-in automatic variable in PowerShell that contains undeclared parameters passed to a function. Using it as a custom variable can cause unexpected behavior or shadowing issues.if ($words.Length -gt 1) { - $args = $words[1..($words.Length - 1)] + $completionArgs = $words[1..($words.Length - 1)] } else { - $args = @() + $completionArgs = @() }And update the usage on line 17:
$allCompletions = ek __complete $completionArgs 2>$nullsrc/cmd/update.ts-30-30 (1)
30-30: Remove trailing space in option description.- .option("--ispersonal", "Make the secret personal ") + .option("--ispersonal", "Make the secret personal")src/cli.ts-31-36 (1)
31-36: Completion logic confirms name inconsistency.Line 33 uses "ek" when building the completion words array, but the program name is configured as "kk" on line 25. This further indicates a naming inconsistency that should be resolved.
src/cli.ts-24-27 (1)
24-27: Fix CLI command name from "kk" to "ek".Line 25 sets
.name("kk")but this conflicts with the completion files (ek.bash, ek.zsh, ek.ps1), the getCompletions call on line 33 which uses "ek", and all README documentation examples. The program name should be "ek".src/providers/enkryptfiy/httpClient.ts-1-17 (1)
1-17: Rename directory to correct the spelling typo across the codebase.The directory "enkryptfiy" is misspelled and should be "enkryptify". However, this typo is consistently referenced throughout the codebase in multiple files and the documentation. Renaming the directory will require updating all import paths in:
- src/providers/registry/index.ts
- src/providers/enkryptfiy/provider.ts
- src/providers/enkryptfiy/auth.ts
- README.md (multiple references)
This should be done as a coordinated change to avoid breaking imports.
src/providers/gcp/auth.ts-15-16 (1)
15-16: Remove debug logging of options parameter.Logging the
optionsobject may expose sensitive information (e.g., keys) in production logs. Consider removing this or logging only non-sensitive fields.async login(options?: LoginOptions): Promise<void> { - console.log("Logging in to Google Cloud...", options); + console.log("Logging in to Google Cloud...");src/providers/aws/auth.ts-9-10 (1)
9-10: Remove debug logging of options parameter.Same as GcpAuth - logging the
optionsobject may expose sensitive information like keys.async login(options?: LoginOptions): Promise<void> { - console.log("Logging in to AWS...", options); + console.log("Logging in to AWS...");src/providers/onePassword/auth.ts-19-23 (1)
19-23: Inconsistent integration metadata with provider.ts.The integration name here is
"My 1Password Integration"with version"v1.0.0", but insrc/providers/onePassword/provider.ts(lines 98-102), the same client creation uses"Enkryptify"and"1.0.0". This inconsistency can confuse 1Password's audit logs and telemetry.Apply this diff to align with the provider.ts values:
const client = await createClient({ auth: token, - integrationName: "My 1Password Integration", - integrationVersion: "v1.0.0", + integrationName: "Enkryptify", + integrationVersion: "1.0.0", });Better yet, extract these to shared constants:
// src/providers/onePassword/constants.ts export const INTEGRATION_NAME = "Enkryptify"; export const INTEGRATION_VERSION = "1.0.0";Then import and use them in both files.
src/providers/enkryptfiy/provider.ts-229-243 (1)
229-243: Missing input validation for secret name.Unlike
updateSecretwhich validates the name pattern (/^[A-Za-z0-9_-]+$/),createSecretdoes not validate thenameparameter. This inconsistency could lead to API errors or invalid secret names being created.async createSecret(config: ProjectConfig, name: string, value: string): Promise<void> { const { workspace_slug, project_slug, environment_id } = this.checkProjectConfig(config); + if (!name || !name.trim()) { + throw new Error("Secret name is required. Please provide a secret name"); + } + + const namePattern = /^[A-Za-z0-9_-]+$/; + if (!namePattern.test(name)) { + throw new Error( + `Invalid secret name "${name}". Name can only contain A-Z, a-z, 0-9, underscore (_), and hyphen (-).`, + ); + } + await http.post(`/v1/workspace/${workspace_slug}/project/${project_slug}/secret`, {src/providers/gcp/provider.ts-78-78 (1)
78-78: Remove debug statement.This
console.logappears to be a debug statement that should be removed before merging.- console.log(options?.env); return await this.getAllSecretsWithAllVersions(projectId, options?.env);src/providers/gcp/provider.ts-41-51 (1)
41-51: Fragile project ID parsing when display name contains "/".If a project's display name contains a "/" character,
selected.split("/").pop()will incorrectly extract part of the name instead of the project ID.Consider using a separator that's invalid in both names and IDs, or storing the mapping separately:
const labels = projects.map((p) => { const name = p.displayName ?? p.name ?? "Unnamed project"; const id = p.projectId ?? "unknown-id"; - return `${name}/${id}`; + return `${name} (${id})`; }); const selected = await selectName(labels, "Select project"); if (!selected) throw new Error("Failed to select project"); - const selectedId = selected.split("/").pop(); + const match = selected.match(/\(([^)]+)\)$/); + const selectedId = match?.[1]; if (!selectedId) throw new Error("Failed to parse selected project id");
🧹 Nitpick comments (36)
.prettierrc.json (1)
7-14: Consider removing package.json from requirePragma.While
requirePragma: trueis appropriate forbun.lock(lock files shouldn't be auto-formatted), applying it topackage.jsonmay prevent useful automatic formatting.package.jsontypically benefits from consistent formatting, and requiring a pragma comment adds friction.Consider this alternative:
"overrides": [ { - "files": ["bun.lock", "package.json"], + "files": ["bun.lock"], "options": { "requirePragma": true } } ]src/ui/SelectItem.tsx (1)
4-31: LGTM with an optional suggestion.The implementation is correct and the use of ANSI escape codes (
\x1b[2J\x1b[H) for clearing the terminal is a common pattern that works on modern terminals.For better cross-platform compatibility and readability, consider using a terminal utility library:
+import { clearTerminal } from "@/lib/terminal"; export async function selectName(options: string[], title?: string): Promise<string> { // ... return new Promise((resolve) => { const select = render( // ... <SelectInput items={items} onSelect={(item) => { select.unmount(); - process.stdout.write("\x1b[2J\x1b[H"); + clearTerminal(); resolve(item.value as string); }} /> // ... ); }); }src/lib/inject.ts (2)
3-35: Good security-conscious approach to protecting critical environment variables.The list covers important categories: PATH manipulation, dynamic linker injection (LD_PRELOAD, DYLD_*), language-specific paths, shell behavior (IFS, BASH_ENV), and user identity. Consider also including temp directory variables (
TMPDIR,TEMP,TMP) which could enable temp directory hijacking attacks.
47-64: Consider validatingsecret.valuebefore assignment.The code validates
secret.namebut assignssecret.valuedirectly. Ifvalueisundefined, this could result in unexpected behavior when the environment variable is read later.for (const secret of secrets) { if (!secret?.name) continue; + if (secret.value === undefined || secret.value === null) continue; if (isDangerousEnvVar(secret.name)) {src/lib/keyring.ts (1)
12-15: Inconsistent error handling between methods.
set()propagates exceptions whileget()anddelete()swallow them with warnings. This inconsistency could surprise callers. Consider either wrappingset()in try/catch for consistency, or letting all methods propagate errors and handling them at the call site.async set(key: string, value: string): Promise<void> { + try { await keytar.setPassword(SERVICE_NAME, key, value); + } catch (error: unknown) { + console.warn(error instanceof Error ? error.message : String(error)); + throw error; // Re-throw so caller knows it failed + } }.github/workflows/release.yml (2)
235-239: Token exposed in git push URL may appear in logs.Embedding the token in the URL (
https://${GH_TOKEN}@github.com/...) can expose it in error messages or verbose logs. Consider using environment-based authentication instead.- git push "https://${GH_TOKEN}@github.com/enkryptify/${BREW_REPO}.git" + git remote set-url origin "https://x-access-token:${GH_TOKEN}@github.com/enkryptify/${BREW_REPO}.git" + git push origin HEADAlternatively, use
gh auth setup-gitif the GitHub CLI is available.
277-281: Same token exposure concern as Homebrew step.The git push URL pattern has the same issue with potential token exposure. Consider applying the same fix here.
- git push "https://${GH_TOKEN}@github.com/Enkryptify/${SCOOP_REPO}.git" + git remote set-url origin "https://x-access-token:${GH_TOKEN}@github.com/Enkryptify/${SCOOP_REPO}.git" + git push origin HEADsrc/providers/registry/ProviderRegistry.ts (1)
6-8: Consider validating provider registration.The
registermethod could validate that the provider name is not empty and that duplicate registrations are intentional. However, this is optional and can be deferred if the current behavior is acceptable.Example enhancement:
register(provider: Provider): void { if (!provider.name || provider.name.trim().length === 0) { throw new Error("Provider name cannot be empty"); } if (this.providers.has(provider.name)) { console.warn(`Provider "${provider.name}" is already registered and will be overwritten`); } this.providers.set(provider.name, provider); }src/lib/error.ts (1)
1-5: Consider using a color library for better terminal compatibility.The hardcoded ANSI escape sequences work but don't respect
NO_COLORconventions or handle non-TTY terminals. Libraries likepicocolorsorchalkprovide cross-platform color support with automatic TTY detection.Example with picocolors:
import pc from "picocolors"; export function logError(message: string): void { console.error(pc.red(`⚠️ ${message}`)); }src/ui/Confirm.tsx (1)
23-23: Consider safer screen clearing approach.The hardcoded ANSI escape sequence
\x1b[2J\x1b[H]may not work correctly on all terminals. Consider checkingprocess.stdout.isTTYor using a library likeansi-escapesfor cross-platform terminal control.Example:
import ansiEscapes from "ansi-escapes"; // In onSelect: confirm.unmount(); if (process.stdout.isTTY) { process.stdout.write(ansiEscapes.clearScreen); } resolve(item.value === "yes");src/cmd/listCommand.ts (1)
7-22: Consider adding null check forprojectConfigbefore accessing.provider.If
config.findProjectConfig()returnsnullorundefined(e.g., when no config file exists), accessingprojectConfig.providerwill throw an unclear error. Consider adding explicit validation.export async function ListSecretsCommand(mode: "show" | "hide"): Promise<void> { const projectConfig = await config.findProjectConfig(process.cwd()); + if (!projectConfig) { + throw new Error("No project configuration found. Run 'ek configure' to set up a project."); + } const provider = providerRegistry.get(projectConfig.provider);src/cmd/update.ts (1)
6-23: Good input validation pattern.The name validation and provider resolution logic is well-structured. Consider extracting the repeated provider resolution pattern (lines 13-20) into a shared utility, as it's duplicated across
listCommand.tsand other command modules.A shared helper could reduce duplication:
// In a shared module export function getProviderOrThrow(providerName: string) { const provider = providerRegistry.get(providerName); if (!provider) { const available = providerRegistry.list().map((p) => p.name).join(", "); throw new Error(`Provider "${providerName}" not found. Available providers: ${available}`); } return provider; }src/providers/enkryptfiy/httpClient.ts (1)
19-46: Consider using centralized error logging.The request interceptor uses
console.warnfor error logging, while the rest of the codebase uses the centralizedlogErrorutility (fromsrc/lib/error.ts). For consistency, consider using the centralized logging approach.Apply this diff:
+import { logError } from "@/lib/error"; + http.interceptors.request.use( async (config) => { try { const authDataString = await keyring.get("enkryptify"); // ... rest of logic } catch (error) { - console.warn("Failed to retrieve auth token:", error instanceof Error ? error.message : String(error)); + logError(`Failed to retrieve auth token: ${error instanceof Error ? error.message : String(error)}`); }src/ui/GcpLogin.tsx (1)
1-36: Consider abstracting the common login pattern.This component follows an identical pattern to other provider login UI components (AwsLogin, EnkryptifyLogin, OnePasswordLogin). The core logic—status management, async login flow, error handling, and process.nextTick for completion callbacks—is duplicated across multiple files.
Consider creating a generic
ProviderLoginUIcomponent that accepts provider-specific messages as props:interface ProviderLoginUIProps { provider: Provider; options?: LoginOptions; providerDisplayName: string; loadingMessage: string; successMessage: string; helpText: string; onError?: (error: Error) => void; onComplete?: () => void; }This would reduce duplication and make the codebase more maintainable.
src/cmd/login.ts (1)
40-44: Consider moving provider-specific prompting logic into providers.The OnePassword-specific token prompting logic creates tight coupling between the login command and a specific provider implementation. If other providers require similar pre-login prompts, this approach doesn't scale well.
Consider having the provider's
loginmethod handle its own authentication prompts, or implement a pre-login hook pattern:interface Provider { login(options?: LoginOptions): Promise<void>; // Optional: providers can implement this to gather credentials before UI flow prepareLoginOptions?(options?: LoginOptions): Promise<LoginOptions>; }This would keep provider-specific logic encapsulated within provider implementations.
src/ui/SecretsTable.tsx (1)
31-43: Consider using String.prototype.padEnd for cleaner padding.The manual padding calculation works correctly but could be simplified using the built-in
padEndmethod.Apply this diff:
- const truncatedValue = truncateValue(secret.value, valueMaxLength); - const namePadding = " ".repeat(Math.max(0, nameColumnWidth - secret.name.length)); - return ( <Box key={secret.id || index} flexDirection="column"> <Box flexDirection="row" flexWrap="wrap"> <Box minWidth={nameColumnWidth}> - <Text> - {secret.name} - {namePadding} - </Text> + <Text>{secret.name.padEnd(nameColumnWidth)}</Text> </Box> <Box flexGrow={1} minWidth={1}> - <Text wrap="wrap">{truncatedValue}</Text> + <Text wrap="wrap">{truncateValue(secret.value, valueMaxLength)}</Text> </Box>src/complete/install-completions.sh (2)
24-44: Simplify the conditional logic for better readability.The installation logic uses complex conditional expressions and an unusual
{ fallback } && success_msgpattern that makes the control flow difficult to follow.Consider restructuring for clarity:
if [ "$SHELL_TYPE" = "bash" ]; then - # Try system-wide location first (requires sudo) - if [ -d "/etc/bash_completion.d" ] && [ -w "/etc/bash_completion.d" ] || [ "$EUID" -eq 0 ]; then - INSTALL_DIR="/etc/bash_completion.d" - sudo cp "$COMPLETIONS_DIR/ek.bash" "$INSTALL_DIR/ek" 2>/dev/null || { - # Fall back to user location - USER_DIR="$HOME/.local/share/bash-completion/completions" - mkdir -p "$USER_DIR" - cp "$COMPLETIONS_DIR/ek.bash" "$USER_DIR/ek" - echo "Bash completion installed to $USER_DIR/ek" - echo "Note: You may need to restart your shell or run: source ~/.bashrc" - } && echo "Bash completion installed to $INSTALL_DIR/ek" - else - # User location + # Try system-wide installation if running as root and directory exists + if [ "$EUID" -eq 0 ] && [ -d "/etc/bash_completion.d" ]; then + cp "$COMPLETIONS_DIR/ek.bash" "/etc/bash_completion.d/ek" + echo "Bash completion installed to /etc/bash_completion.d/ek" + elif [ -w "/etc/bash_completion.d" ]; then + cp "$COMPLETIONS_DIR/ek.bash" "/etc/bash_completion.d/ek" + echo "Bash completion installed to /etc/bash_completion.d/ek" + else + # Fall back to user directory USER_DIR="$HOME/.local/share/bash-completion/completions" mkdir -p "$USER_DIR" cp "$COMPLETIONS_DIR/ek.bash" "$USER_DIR/ek" echo "Bash completion installed to $USER_DIR/ek" echo "Note: You may need to restart your shell or run: source ~/.bashrc" fi fi
46-69: Apply the same simplification to zsh installation.The zsh installation block has the same complex conditional logic as the bash section. The same refactoring approach recommended for bash (lines 24-44) should be applied here for consistency and readability.
src/providers/gcp/auth.ts (1)
41-43: Stub method should be documented or tracked.The
getCredentials()method throws "Method not implemented" which satisfies the interface but will cause runtime errors if called. Consider adding a TODO comment or tracking this for implementation.getCredentials(): Promise<Credentials> { + // TODO: Implement credential retrieval for GCP throw new Error("Method not implemented."); }src/providers/aws/auth.ts (1)
27-27: Chain the original error for better debugging.Unlike GcpAuth, the AWS error doesn't preserve the original error as the cause. This makes debugging harder. Consider consistency with the GCP implementation.
- throw new Error("AWS authentication failed"); + const message = err instanceof Error ? err.message : String(err); + throw new Error(`AWS authentication failed: ${message}`, { cause: err });src/ui/EnkryptifyLogin.tsx (1)
66-70: Browser authentication hint may not apply to all providers.The message "Please complete authentication in your browser..." is hardcoded, but Enkryptify authentication may not always involve a browser flow. Consider making this message configurable or provider-specific.
src/ui/AwsLogin.tsx (2)
14-74: Consider extracting a shared login component.
AwsLoginandEnkryptifyLoginshare nearly identical logic with only message text differences. Consider creating a genericLoginUIcomponent that accepts the provider name and custom messages as props to reduce duplication.// Example shared component signature: interface LoginUIProps { provider: Provider; providerDisplayName: string; loadingHint?: string; options?: LoginOptions; onError?: (error: Error) => void; onComplete?: () => void; } export function LoginUI({ provider, providerDisplayName, loadingHint, ... }: LoginUIProps) { // Shared login logic }
25-27: Inconsistent completion callback timing.
AwsLoginusesprocess.nextTick()for the completion callback whileEnkryptifyLoginusessetTimeout(..., 1000). This leads to inconsistent UX across providers. Consider standardizing the approach.src/cmd/create.ts (2)
27-29: Redundant validation - value already validated in command handler.The
createSecretCommandfunction validates thatvalueis non-empty, but the command handler at lines 48-53 already ensures a non-empty value before calling this function. Consider removing this redundant check or documenting thatcreateSecretCommandcan be called directly (where validation would be needed).
48-53: Redundant post-prompt validation.The
getSecureInputfunction already validates non-empty input (per the relevant snippet showingvalidate: (value) => ...). The check on lines 50-52 is redundant unlessgetSecureInputbehavior changes.if (!secretValue || secretValue.trim().length === 0) { secretValue = await getSecureInput("Enter secret value: "); - if (!secretValue || secretValue.trim().length === 0) { - throw new Error("Secret value cannot be empty please provide a value."); - } }src/providers/onePassword/provider.ts (1)
14-29: Potential robustness issue with dotenv.parse.Using
dotenv.parsefor a single line may not handle edge cases correctly (e.g., quotes, escapes, multiline values). Since you're storingKEY=VALUEpairs in a controlled format, consider a simpler split approach or enforce strict formatting.function parseEnvLine(line: string): ParsedNote | null { const match = line.match(/^([A-Za-z_][A-Za-z0-9_]*)=(.*)$/); if (!match) return null; return { key: match[1], value: match[2] }; }This avoids the dependency on
dotenvfor parsing and enforces valid environment variable naming.src/providers/aws/provider.ts (1)
79-119: Consider handling paginated ListSecrets failures gracefully.If
ListSecretsCommandfails mid-pagination (e.g., throttling, network issue), the entirerunoperation fails. For better resilience, consider logging the error and returning partial results, or implementing retry logic for transient failures.src/providers/enkryptfiy/provider.ts (5)
1-10: Directory name typo: "enkryptfiy" should be "enkryptify".The directory is misspelled as
enkryptfiyinstead ofenkryptify. This inconsistency will cause confusion and should be corrected before the codebase grows.
106-109: Unnecessary template literal.The template literal wrapping
ws.slugis redundant since it's already a string.const workspaceSlug = await selectName( - workspaces.map((ws) => `${ws.slug}`), + workspaces.map((ws) => ws.slug), "Select workspace", );
207-224: Duplicated secret mapping logic.The secret value extraction and mapping logic (lines 207-224) is nearly identical to the logic in
listSecrets(lines 65-80). Consider extracting this into a private helper method to reduce duplication and ensure consistent behavior.Example helper:
private mapApiSecretsToSecrets(apiSecrets: ApiSecret[], environmentId: string, maskValues = false): Secret[] { return apiSecrets.map((secret) => { const personalValue = secret.values.find( (v) => v.environmentId === environmentId && v.isPersonal === true, ); const nonPersonalValue = secret.values.find( (v) => v.environmentId === environmentId && v.isPersonal === false, ); const matching = personalValue || nonPersonalValue; return { id: secret.id, name: secret.name, value: maskValues && !matching ? "*********" : (matching?.value ?? ""), isPersonal: matching?.isPersonal ?? false, environmentId: maskValues && !matching ? "*********" : (matching?.environmentId ?? ""), }; }); }
366-369: Redundant nullish coalescing.Since
statusis checked for truthiness in theifcondition,status ?? "unknown"will never evaluate to"unknown".if (error instanceof AxiosError) { const status = error.response?.status; - if (status) throw new Error(`Failed to fetch resources from ${url}. Status: ${status ?? "unknown"}.`); + if (status) throw new Error(`Failed to fetch resources from ${url}. Status: ${status}.`); }
374-382: Typo in error message and method visibility.
- The error message contains "pls" which should be "please" for professional messaging.
- This method appears to be an internal helper and should be marked as
private.- checkProjectConfig(config: ProjectConfig) { + private checkProjectConfig(config: ProjectConfig) { const { workspace_slug, project_slug, environment_id } = config; if (!workspace_slug || !project_slug || !environment_id) { throw new Error( - "Invalid config: missing workspace_slug, project_slug, or environment_id pls run ek setup first", + "Invalid config: missing workspace_slug, project_slug, or environment_id. Please run 'ek setup' first.", ); } return { workspace_slug, project_slug, environment_id }; }src/providers/enkryptfiy/auth.ts (2)
54-55: Type coercion in authentication check.
getUserInforeturnsUserInfo | null, butisAuthis checked as a boolean. While this works due to JavaScript truthiness, using an explicit check would be clearer.- const isAuth = await this.getUserInfo(envToken).catch(() => false); - if (isAuth) { + const userInfo = await this.getUserInfo(envToken).catch(() => null); + if (userInfo) {
152-156: No error handling for port conflict.If port 51823 is already in use,
Bun.servewill fail. Consider wrapping this in a try-catch with a descriptive error message guiding the user to check if another process is using the port.+ try { server = Bun.serve({ port: self.CALLBACK_PORT, routes: { "/callback": handleCallback }, fetch: () => new Response("Not Found", { status: 404 }), }); + } catch (err) { + fail(new Error(`Failed to start local auth server on port ${self.CALLBACK_PORT}. Ensure the port is not in use.`)); + return; + }src/providers/gcp/provider.ts (2)
223-251: Performance concern: fetching all versions of all secrets.This method makes O(secrets × versions) API calls, which can be very slow and expensive. For most use cases (especially
listSecretsandrun), only the latest version of each secret is needed.Consider adding a
latestOnlyparameter or creating a separate method that fetches only the latest version:async getLatestSecrets(projectId: string): Promise<Secret[]> { const parent = `projects/${projectId}`; const [secrets] = await this.secretsClient.listSecrets({ parent }); const results: Secret[] = []; for (const meta of secrets) { if (!meta.name) continue; const secretId = meta.name.split("/").pop() ?? meta.name; // Access only "latest" version const [version] = await this.secretsClient.accessSecretVersion({ name: `${meta.name}/versions/latest`, }); const value = version.payload?.data instanceof Uint8Array ? Buffer.from(version.payload.data).toString("utf8") : ""; results.push({ name: secretId, value }); } return results; }
189-204: Consider using latest version only forlistSecrets.
listSecretscallsgetAllSecretsWithAllVersionswhich fetches every version of every secret. For a "list" operation, typically only the latest version is relevant. This compounds the performance issue noted above.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/.DS_Storeis excluded by!**/.DS_Storebun.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (72)
.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks).goreleaser.yaml(0 hunks).prettierrc.json(1 hunks).vscode/settings.json(1 hunks)CLAUDE.md(1 hunks)LICENSE(2 hunks)README.md(1 hunks)cmd/login.go(0 hunks)cmd/root.go(0 hunks)cmd/run.go(0 hunks)cmd/setup.go(0 hunks)eslint.config.ts(1 hunks)go.mod(0 hunks)install.sh(0 hunks)internal/auth/enkryptify.go(0 hunks)internal/config/config.go(0 hunks)internal/config/setup.go(0 hunks)internal/inject/env.go(0 hunks)internal/keyring/keyring.go(0 hunks)internal/providers/enkryptify/provider.go(0 hunks)internal/ui/login.go(0 hunks)internal/ui/selection.go(0 hunks)internal/ui/ui.go(0 hunks)main.go(0 hunks)package.json(1 hunks)src/cli.ts(1 hunks)src/cmd/configure.ts(1 hunks)src/cmd/create.ts(1 hunks)src/cmd/delete.ts(1 hunks)src/cmd/index.ts(1 hunks)src/cmd/listCommand.ts(1 hunks)src/cmd/login.ts(1 hunks)src/cmd/run.ts(1 hunks)src/cmd/update.ts(1 hunks)src/complete/complete.ts(1 hunks)src/complete/ek.bash(1 hunks)src/complete/ek.ps1(1 hunks)src/complete/ek.zsh(1 hunks)src/complete/install-completions.sh(1 hunks)src/env.ts(1 hunks)src/lib/config.ts(1 hunks)src/lib/error.ts(1 hunks)src/lib/inject.ts(1 hunks)src/lib/input.ts(1 hunks)src/lib/keyring.ts(1 hunks)src/lib/terminal.ts(1 hunks)src/providers/aws/auth.ts(1 hunks)src/providers/aws/provider.ts(1 hunks)src/providers/base/AuthProvider.ts(1 hunks)src/providers/base/Provider.ts(1 hunks)src/providers/enkryptfiy/auth.ts(1 hunks)src/providers/enkryptfiy/httpClient.ts(1 hunks)src/providers/enkryptfiy/provider.ts(1 hunks)src/providers/gcp/auth.ts(1 hunks)src/providers/gcp/provider.ts(1 hunks)src/providers/onePassword/auth.ts(1 hunks)src/providers/onePassword/provider.ts(1 hunks)src/providers/registry/ProviderRegistry.ts(1 hunks)src/providers/registry/index.ts(1 hunks)src/ui/AwsLogin.tsx(1 hunks)src/ui/Confirm.tsx(1 hunks)src/ui/EnkryptifyLogin.tsx(1 hunks)src/ui/GcpLogin.tsx(1 hunks)src/ui/LoginFlow.tsx(1 hunks)src/ui/RunFlow.tsx(1 hunks)src/ui/SecretsTable.tsx(1 hunks)src/ui/SelectItem.tsx(1 hunks)src/ui/SuccessMessage.tsx(1 hunks)src/ui/onePassword.tsx(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (17)
- .goreleaser.yaml
- install.sh
- internal/ui/login.go
- go.mod
- cmd/setup.go
- main.go
- internal/inject/env.go
- cmd/run.go
- internal/config/config.go
- internal/ui/ui.go
- internal/providers/enkryptify/provider.go
- internal/config/setup.go
- cmd/root.go
- internal/keyring/keyring.go
- internal/auth/enkryptify.go
- cmd/login.go
- internal/ui/selection.go
🧰 Additional context used
🧬 Code graph analysis (29)
src/ui/onePassword.tsx (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)
src/ui/GcpLogin.tsx (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)
src/ui/SecretsTable.tsx (1)
src/providers/base/Provider.ts (1)
Secret(4-10)
src/cmd/listCommand.ts (5)
src/lib/config.ts (1)
config(222-228)src/providers/registry/index.ts (1)
providerRegistry(11-11)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/ui/SecretsTable.tsx (1)
showSecretsTable(61-71)src/lib/error.ts (1)
logError(1-5)
src/providers/gcp/auth.ts (2)
src/providers/base/AuthProvider.ts (3)
AuthProvider(11-15)LoginOptions(5-9)Credentials(1-3)src/lib/config.ts (1)
config(222-228)
src/cmd/index.ts (7)
src/cmd/login.ts (1)
registerLoginCommand(7-66)src/cmd/configure.ts (1)
registerConfigureCommand(42-57)src/cmd/run.ts (1)
registerRunCommand(44-69)src/cmd/listCommand.ts (1)
registerListCommand(24-40)src/cmd/create.ts (1)
registerCreateCommand(36-61)src/cmd/delete.ts (1)
registerDeleteCommand(25-39)src/cmd/update.ts (1)
registerUpdateCommand(25-40)
src/cmd/configure.ts (5)
src/providers/aws/provider.ts (1)
configure(25-78)src/providers/enkryptfiy/provider.ts (1)
configure(89-178)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/lib/config.ts (1)
config(222-228)src/lib/error.ts (1)
logError(1-5)
src/cmd/create.ts (6)
src/lib/config.ts (1)
config(222-228)src/providers/registry/index.ts (1)
providerRegistry(11-11)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/ui/SuccessMessage.tsx (1)
showMessage(11-17)src/lib/input.ts (1)
getSecureInput(3-21)src/lib/error.ts (1)
logError(1-5)
src/cmd/delete.ts (4)
src/lib/config.ts (1)
config(222-228)src/providers/registry/index.ts (1)
providerRegistry(11-11)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/lib/error.ts (1)
logError(1-5)
src/ui/EnkryptifyLogin.tsx (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)
src/providers/aws/auth.ts (2)
src/providers/base/AuthProvider.ts (3)
AuthProvider(11-15)LoginOptions(5-9)Credentials(1-3)src/lib/config.ts (1)
config(222-228)
src/cmd/run.ts (5)
src/lib/config.ts (2)
ProjectConfig(7-11)config(222-228)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/lib/inject.ts (1)
buildEnvWithSecrets(44-67)src/ui/RunFlow.tsx (1)
RunFlow(20-40)src/lib/error.ts (1)
logError(1-5)
src/providers/enkryptfiy/httpClient.ts (2)
src/env.ts (1)
env(4-11)src/lib/keyring.ts (1)
keyring(41-41)
src/providers/onePassword/auth.ts (3)
src/providers/base/AuthProvider.ts (3)
AuthProvider(11-15)LoginOptions(5-9)Credentials(1-3)src/lib/keyring.ts (1)
keyring(41-41)src/lib/config.ts (1)
config(222-228)
src/lib/config.ts (3)
src/lib/error.ts (1)
logError(1-5)src/providers/registry/index.ts (1)
providerRegistry(11-11)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)
src/cmd/update.ts (4)
src/lib/config.ts (1)
config(222-228)src/providers/registry/index.ts (1)
providerRegistry(11-11)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/lib/error.ts (1)
logError(1-5)
src/ui/AwsLogin.tsx (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)
src/cli.ts (5)
src/lib/error.ts (1)
logError(1-5)src/env.ts (1)
env(4-11)src/cmd/index.ts (1)
registerCommands(10-18)src/complete/complete.ts (1)
getCompletions(7-22)src/lib/terminal.ts (1)
setupTerminalCleanup(5-26)
src/ui/LoginFlow.tsx (7)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)src/providers/onePassword/auth.ts (1)
login(11-35)src/providers/aws/provider.ts (1)
login(262-264)src/providers/enkryptfiy/provider.ts (1)
login(85-87)src/providers/gcp/auth.ts (1)
login(15-39)src/providers/enkryptfiy/auth.ts (1)
login(39-82)
src/lib/inject.ts (2)
src/providers/base/Provider.ts (1)
Secret(4-10)src/env.ts (1)
env(4-11)
src/ui/RunFlow.tsx (1)
src/providers/enkryptfiy/provider.ts (1)
run(180-227)
src/providers/registry/index.ts (1)
src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)
src/providers/registry/ProviderRegistry.ts (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/registry/index.ts (1)
providerRegistry(11-11)
src/providers/enkryptfiy/auth.ts (3)
src/providers/base/AuthProvider.ts (3)
AuthProvider(11-15)LoginOptions(5-9)Credentials(1-3)src/lib/keyring.ts (1)
keyring(41-41)src/env.ts (1)
env(4-11)
src/providers/gcp/provider.ts (7)
src/providers/base/Provider.ts (3)
Provider(22-38)runOptions(12-15)Secret(4-10)src/providers/gcp/auth.ts (1)
GcpAuth(5-44)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)src/lib/config.ts (2)
ProjectConfig(7-11)config(222-228)src/ui/Confirm.tsx (1)
confirm(4-31)src/ui/SelectItem.tsx (1)
selectName(4-31)src/lib/input.ts (1)
getSecureInput(3-21)
src/providers/enkryptfiy/provider.ts (8)
src/providers/base/Provider.ts (3)
Provider(22-38)Secret(4-10)runOptions(12-15)src/providers/enkryptfiy/auth.ts (1)
EnkryptifyAuth(32-298)src/lib/config.ts (2)
config(222-228)ProjectConfig(7-11)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)src/ui/Confirm.tsx (1)
confirm(4-31)src/ui/SelectItem.tsx (1)
selectName(4-31)src/ui/SuccessMessage.tsx (1)
showMessage(11-17)src/lib/input.ts (2)
getTextInput(23-31)getSecureInput(3-21)
src/providers/base/Provider.ts (2)
src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)src/lib/config.ts (2)
ProjectConfig(7-11)config(222-228)
src/cmd/login.ts (5)
src/providers/registry/index.ts (1)
providerRegistry(11-11)src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)src/lib/error.ts (1)
logError(1-5)src/lib/input.ts (1)
getSecureInput(3-21)src/ui/LoginFlow.tsx (1)
LoginFlow(47-90)
src/providers/onePassword/provider.ts (7)
src/lib/config.ts (2)
config(222-228)ProjectConfig(7-11)src/providers/registry/index.ts (1)
OnePasswordProvider(11-11)src/providers/base/Provider.ts (3)
Provider(22-38)runOptions(12-15)Secret(4-10)src/providers/onePassword/auth.ts (1)
OnePasswordAuth(6-53)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)src/ui/SuccessMessage.tsx (1)
showMessage(11-17)src/lib/input.ts (2)
getTextInput(23-31)getSecureInput(3-21)
🪛 actionlint (1.7.9)
.github/workflows/release.yml
135-135: shellcheck reported issue in this script: SC2035:info:3:11: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
168-168: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 markdownlint-cli2 (0.18.1)
README.md
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
576-576: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
670-670: Spaces inside code span elements
(MD038, no-space-in-code)
🪛 Shellcheck (0.11.0)
src/complete/ek.bash
[warning] 4-4: cur appears unused. Verify use (or export if used externally).
(SC2034)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
src/ui/SelectItem.tsx (2)
22-26: Optional: Remove unnecessary type cast.The
as stringcast on line 25 is redundant sinceitem.valueis already typed asstringbased on theitemsarray definition.Apply this diff to simplify:
<SelectInput items={items} onSelect={(item) => { select.unmount(); restoreCursor(); - resolve(item.value as string); + resolve(item.value); }} />
11-31: Consider adding error handling for the render call.If the
rendercall throws an exception, the Promise will remain unresolved. Consider wrapping the render in a try-catch block to reject the Promise on errors, ensuring callers can handle failures gracefully.Example approach:
return new Promise((resolve, reject) => { try { const select = render( // ... existing JSX ); } catch (error) { reject(error); } });src/lib/terminal.ts (1)
5-27: Optional: Consider making setupTerminalCleanup idempotent.If
setupTerminalCleanupis called multiple times, it will register duplicate event handlers. While this is unlikely in typical usage (usually called once at startup), you could prevent this by tracking whether cleanup has already been set up.Example approach:
let cleanupRegistered = false; export function setupTerminalCleanup(): void { if (cleanupRegistered) return; cleanupRegistered = true; const cleanup = () => { restoreCursor(); }; // ... rest of implementation }test.ts (1)
1-2: Consider removing this test file or clarifying its purpose.This file logs undefined environment variables (
coco,devName) and appears to be debug code or a leftover test artifact with no clear integration into the CLI.If this is temporary test code, remove it before merging. If it serves a specific purpose, consider:
- Adding a comment explaining its role
- Using more descriptive environment variable names
- Integrating it into a proper test suite
-console.log(process.env.coco); -console.log(process.env.devName);src/providers/onePassword/provider.ts (1)
14-29: Consider simplifying single-line KEY=VALUE parsing.Using
dotenv.parse()for single-line parsing works but is heavyweight. A simple regex or split would be more direct and avoid the dependency for this use case.If you prefer to keep the robustness of dotenv parsing (handles quotes, escapes, etc.), the current approach is fine. Otherwise:
function parseEnvLine(line: string): ParsedNote | null { const match = line.match(/^([^=]+)=(.*)$/); if (!match) return null; const [, key, value] = match; return { key: key.trim(), value: value.trim() }; }src/ui/AwsLogin.tsx (1)
31-33: Consider alignment with EnkryptifyLogin's callback timing.This component uses
process.nextTick()(line 31) while EnkryptifyLogin usessetTimeout(..., 1000)(line 30-32 in EnkryptifyLogin.tsx). Both work, but the inconsistency means AWS success completes immediately while Enkryptify shows the success message for 1 second.Consider standardizing the delay approach across login components for consistent UX:
setTimeout(() => { onCompleteRef.current?.(); }, 1000);src/providers/enkryptify/auth.ts (2)
50-64: Consider simplifying the token validation logic.
getUserInforeturnsUserInfo | null, but the.catch(() => false)converts failures tofalse. This meansisAuthcan beUserInfo,null, orfalse. While functionally correct (all are handled by the truthy check), the mixed types reduce clarity.- const isAuth = await this.getUserInfo(envToken).catch(() => false); - if (isAuth) { + const userInfo = await this.getUserInfo(envToken).catch(() => null); + if (userInfo) {
152-156: Handle port-in-use errors gracefully.If port 51823 is already in use,
Bun.servewill throw an error that isn't caught, causing an unhandled rejection. Consider wrapping the server creation in a try-catch or using a port-finding mechanism.+ try { server = Bun.serve({ port: self.CALLBACK_PORT, routes: { "/callback": handleCallback }, fetch: () => new Response("Not Found", { status: 404 }), }); + } catch (err) { + reject(new Error(`Failed to start callback server on port ${self.CALLBACK_PORT}: ${err instanceof Error ? err.message : String(err)}`)); + return; + }src/providers/aws/provider.ts (1)
80-120: Duplicate logic with listSecrets method.The
runandlistSecretsmethods share nearly identical secret fetching and processing logic (lines 87-119 vs lines 233-265). Consider extracting a private helper method to reduce duplication.private async fetchSecretsWithValues(prefix: string): Promise<Array<{name: string, value: string}>> { const allSecrets = []; let nextToken: string | undefined; do { const res = await this.secretsClient.send(new ListSecretsCommand({ NextToken: nextToken })); allSecrets.push(...(res.SecretList ?? [])); nextToken = res.NextToken; } while (nextToken); const matches = allSecrets.filter((s) => s.Name && s.Name.startsWith(prefix)); return Promise.all( matches.map(async (meta) => { const valueRes = await this.secretsClient.send( new GetSecretValueCommand({ SecretId: meta.ARN ?? meta.Name! }), ); const fullName = meta.Name!; let envName = fullName.slice(prefix.length); envName = envName.replace(/^\/+/, ""); return { name: envName, value: valueRes.SecretString ?? "" }; }), ); }src/providers/enkryptify/provider.ts (3)
73-79: Unusual masking of environmentId.The
environmentIdis masked with"*********"whenshowValuesis not "show". Environment IDs are typically not sensitive and masking them may confuse users or break downstream tooling that depends on this metadata.return { id: secret.id, name: secret.name, value: shouldShow && matching ? (matching.value ?? "") : "*********", isPersonal: matching?.isPersonal ?? false, - environmentId: shouldShow && matching ? (matching.environmentId ?? "") : "*********", + environmentId: matching?.environmentId ?? "", };
366-370: Redundant status check.Line 368 checks
if (status)then usesstatus ?? "unknown". The nullish coalescing is redundant sincestatusis already confirmed to be truthy.if (error instanceof AxiosError) { const status = error.response?.status; - if (status) throw new Error(`Failed to fetch resources from ${url}. Status: ${status ?? "unknown"}.`); + if (status) throw new Error(`Failed to fetch resources from ${url}. Status: ${status}.`); }
374-382: Make method private and fix typo.
checkProjectConfigis only used internally and should be markedprivate. Also, there's a typo in the error message: "pls" should be "please".- checkProjectConfig(config: ProjectConfig) { + private checkProjectConfig(config: ProjectConfig) { const { workspace_slug, project_slug, environment_id } = config; if (!workspace_slug || !project_slug || !environment_id) { throw new Error( - "Invalid config: missing workspace_slug, project_slug, or environment_id pls run ek setup first", + "Invalid config: missing workspace_slug, project_slug, or environment_id. Please run ek setup first.", ); } return { workspace_slug, project_slug, environment_id }; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/release.yml(1 hunks)src/cli.ts(1 hunks)src/cmd/configure.ts(1 hunks)src/cmd/run.ts(1 hunks)src/cmd/update.ts(1 hunks)src/lib/input.ts(1 hunks)src/lib/terminal.ts(1 hunks)src/providers/aws/auth.ts(1 hunks)src/providers/aws/provider.ts(1 hunks)src/providers/enkryptify/auth.ts(1 hunks)src/providers/enkryptify/httpClient.ts(1 hunks)src/providers/enkryptify/provider.ts(1 hunks)src/providers/gcp/auth.ts(1 hunks)src/providers/onePassword/provider.ts(1 hunks)src/providers/registry/index.ts(1 hunks)src/ui/AwsLogin.tsx(1 hunks)src/ui/Confirm.tsx(1 hunks)src/ui/EnkryptifyLogin.tsx(1 hunks)src/ui/SecretsTable.tsx(1 hunks)src/ui/SelectItem.tsx(1 hunks)src/ui/SuccessMessage.tsx(1 hunks)test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/cmd/update.ts
- src/cmd/run.ts
- src/ui/SuccessMessage.tsx
- src/cli.ts
- src/cmd/configure.ts
- src/providers/aws/auth.ts
- src/ui/SecretsTable.tsx
- src/lib/input.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T15:16:30.819Z
Learnt from: alizamili00
Repo: Enkryptify/cli PR: 15
File: src/providers/gcp/provider.ts:206-212
Timestamp: 2025-12-17T15:16:30.819Z
Learning: In the Enkryptify CLI codebase, the `env` parameter in provider methods (like `runOptions`) is intentionally designed to be flexible across providers: it can represent an environment name (e.g., "staging", "production") for some providers, or a project identifier (like a GCP project ID or AWS project ID) for others, depending on provider-specific requirements.
Applied to files:
src/providers/enkryptify/auth.tssrc/providers/enkryptify/provider.ts
🧬 Code graph analysis (8)
src/providers/enkryptify/auth.ts (3)
src/providers/base/AuthProvider.ts (3)
AuthProvider(11-15)LoginOptions(5-9)Credentials(1-3)src/lib/keyring.ts (1)
keyring(41-41)src/env.ts (1)
env(4-11)
src/providers/registry/index.ts (1)
src/providers/registry/ProviderRegistry.ts (1)
providerRegistry(23-23)
src/providers/gcp/auth.ts (2)
src/providers/base/AuthProvider.ts (3)
AuthProvider(11-15)LoginOptions(5-9)Credentials(1-3)src/lib/config.ts (1)
config(222-228)
src/providers/enkryptify/httpClient.ts (2)
src/env.ts (1)
env(4-11)src/lib/keyring.ts (1)
keyring(41-41)
src/providers/enkryptify/provider.ts (6)
src/providers/registry/index.ts (1)
EnkryptifyProvider(11-11)src/providers/base/Provider.ts (3)
Provider(22-38)Secret(4-10)runOptions(12-15)src/providers/enkryptify/auth.ts (1)
EnkryptifyAuth(32-304)src/lib/config.ts (2)
config(222-228)ProjectConfig(7-11)src/ui/SelectItem.tsx (1)
selectName(5-32)src/lib/input.ts (2)
getTextInput(23-35)getSecureInput(3-21)
src/ui/AwsLogin.tsx (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)
src/ui/SelectItem.tsx (1)
src/lib/terminal.ts (1)
restoreCursor(1-3)
src/ui/EnkryptifyLogin.tsx (2)
src/providers/base/Provider.ts (1)
Provider(22-38)src/providers/base/AuthProvider.ts (1)
LoginOptions(5-9)
🔇 Additional comments (26)
src/ui/SelectItem.tsx (1)
5-10: LGTM!The function signature is clear, and the item mapping logic correctly creates unique keys by combining name and index, which prevents collisions even if duplicate names exist in the options array.
src/lib/terminal.ts (2)
1-3: LGTM!The ANSI escape sequence correctly shows the terminal cursor.
22-26: Critical issue resolved!The previous infinite recursion bug in the
uncaughtExceptionhandler has been fixed. The current implementation correctly logs the error and exits with code 1 instead of re-throwing, which would have triggered anotheruncaughtExceptionevent.src/ui/Confirm.tsx (1)
1-16: LGTM!The confirm utility is well-implemented with appropriate error handling for user cancellation. The type assertions are safe given the prompt configuration.
src/providers/onePassword/provider.ts (2)
271-307: LGTM! Key-level deletion implemented correctly.The deleteSecret method now supports both key-level and note-level deletion with appropriate user prompts and validation, addressing the previous review feedback.
145-175: LGTM! Graceful error handling for individual note failures.The run method correctly handles per-note fetch failures with warnings (line 166) while continuing to process remaining notes, ensuring partial failures don't break the entire operation.
src/providers/enkryptify/httpClient.ts (2)
19-46: LGTM! Request interceptor handles auth gracefully.The interceptor properly retrieves credentials from the keyring, performs necessary type guards, and gracefully handles missing credentials without breaking requests. The warning on line 40 provides useful debugging information.
55-66: Response interceptor provides good user experience.Mapping common HTTP status codes to user-friendly error messages improves the CLI experience. Unmapped status codes pass through with their original error details.
src/providers/registry/index.ts (1)
1-11: LGTM! Provider registration is correct.All four providers are properly imported, registered with the registry, and exported. The past review concerns about the import path typo and missing EnkryptifyProvider export have been resolved.
src/providers/gcp/auth.ts (1)
9-13: LGTM! Scope configuration fixed.The constructor now properly handles
GCP_SCOPESby splitting on commas and providing a sensible default scope array, addressing the previous review concern about empty string fallbacks.src/ui/EnkryptifyLogin.tsx (2)
16-22: LGTM! Callback dependency issue resolved.The component now uses refs to hold callback references (lines 16-17) and updates them in a separate effect (lines 19-22), preventing infinite re-renders when parent components pass non-memoized callbacks. This addresses the previous review concern.
71-75: Verify browser authentication message accuracy.The message "Please complete authentication in your browser..." (line 73) may not accurately reflect the Enkryptify authentication flow. Verify that Enkryptify authentication actually requires browser interaction.
If Enkryptify uses a different auth flow (e.g., API keys, CLI prompts), update the message accordingly:
{status === "loading" && ( <Box marginTop={1}> - <Text>Please complete authentication in your browser...</Text> + <Text>Authenticating with your Enkryptify credentials...</Text> </Box> )}src/ui/AwsLogin.tsx (2)
17-23: LGTM! Callback dependency issue resolved.The component correctly uses refs to store callbacks (lines 17-18) and updates them in a separate effect (lines 20-23), avoiding the infinite re-render issue identified in the previous review.
72-78: LGTM! Informative loading message for AWS.The message clearly explains that the CLI uses local AWS credentials, setting appropriate user expectations during the authentication process.
src/providers/enkryptify/auth.ts (4)
1-30: LGTM for imports, types, and base64Url helper.The types are well-defined and the base64Url function correctly implements RFC 4648 URL-safe base64 encoding, essential for PKCE.
195-213: XSS mitigation applied correctly.The HTML escaping for special characters addresses the previously identified XSS vulnerability. The implementation properly escapes
&,<,>,", and'.
267-285: LGTM for getUserInfo implementation.The method properly distinguishes authentication failures (401/403 → null) from other errors (thrown), enabling graceful handling of invalid tokens during login flow.
287-303: LGTM with minor observation.The credentials retrieval logic is sound. The nested try-catch could be simplified since both the parse failure and missing accessToken cases throw identical errors, but this is acceptable for clarity.
src/providers/aws/provider.ts (5)
1-14: Imports are complete and correct.The missing
confirmimport from the past review has been added on line 4.
21-24: LGTM for constructor.Using an empty config for
SecretsManagerClientcorrectly leverages the AWS SDK's default credential provider chain.
26-79: LGTM for configure method.The interactive configuration flow handles pagination correctly, validates prefix existence by checking for matching secrets, and provides clear feedback. The retry loop pattern is appropriate for CLI input.
204-216: Permanent deletion without recovery window.
ForceDeleteWithoutRecovery: trueimmediately and permanently deletes the secret with no recovery option. AWS Secrets Manager normally provides a 7-30 day recovery window. Consider whether this aggressive behavior is intentional or if a confirmation prompt should be added.
272-278: LGTM for helper methods.
normalizePrefixandbuildFullNamecorrectly handle slash normalization to prevent malformed secret paths.src/providers/enkryptify/provider.ts (3)
1-46: LGTM for imports and type definitions.The types accurately model the API resources and the imports are well-organized.
89-178: LGTM for configure method.The interactive configuration flow provides a good user experience with clear selection prompts and helpful error messages when workspaces, projects, or environments are missing.
245-326: LGTM for updateSecret method.The implementation correctly handles complex scenarios: preserving values from other environments, managing personal vs non-personal values, validating naming conventions, and checking uniqueness. The logic on lines 296-314 for maintaining existing values while updating the target is well-designed.
| const res = await http.post<AuthResponse>("/v1/auth/token", payload, { | ||
| validateStatus: () => true, | ||
| }); | ||
|
|
||
| if (res.status < 200 || res.status >= 300) { | ||
| const text = typeof res.data === "string" ? res.data : JSON.stringify(res.data); | ||
| throw new Error(`token exchange failed with status ${res.status}: ${text}`); | ||
| } | ||
|
|
||
| const data = res.data as AuthResponse; | ||
| if (!data.accessToken) { | ||
| throw new Error("token exchange response missing accessToken"); | ||
| } | ||
|
|
||
| return data; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for AuthResponse type definition
echo "=== AuthResponse Type Definition ==="
rg "interface AuthResponse|type AuthResponse" -A 10 --type ts
# Search for accessToken/access_token usage
echo -e "\n=== accessToken Usage ==="
rg "accessToken" --type ts -n
echo -e "\n=== access_token Usage ==="
rg "access_token" --type ts -nRepository: Enkryptify/cli
Length of output: 1840
🏁 Script executed:
#!/bin/bash
# Examine the HTTP client to see if there's response transformation
echo "=== HTTP Client Configuration ==="
rg "http\." -B 3 -A 3 --type ts | head -100
# Check for axios interceptors or transformers
echo -e "\n=== Axios/HTTP Transformers or Interceptors ==="
rg "interceptor|transformer|transform" --type ts -iRepository: Enkryptify/cli
Length of output: 6297
🏁 Script executed:
#!/bin/bash
echo "=== Full httpClient.ts Implementation ==="
cat -n src/providers/enkryptify/httpClient.tsRepository: Enkryptify/cli
Length of output: 2479
🏁 Script executed:
#!/bin/bash
echo "=== Full auth.ts file to see complete context ==="
cat -n src/providers/enkryptify/auth.ts | head -260Repository: Enkryptify/cli
Length of output: 11586
Add a response field transformer or document the expected field naming convention.
The code expects accessToken (camelCase) from the token endpoint, but OAuth 2.0 standard responses use access_token (snake_case). While the current implementation works if Enkryptify's API intentionally returns camelCase, this creates fragility and deviates from OAuth 2.0 conventions. Either implement a field transformer (mapping access_token to accessToken) or explicitly document that Enkryptify's API uses camelCase field names.
🤖 Prompt for AI Agents
In src/providers/enkryptify/auth.ts around lines 237 to 252, the token response
validation assumes a camelCase accessToken but OAuth uses access_token; update
the code to accept and normalize both formats: read res.data, if it contains
access_token map it to accessToken (and do the same for refresh_token →
refreshToken if applicable), then validate using the normalized object and throw
the same error message if accessToken is missing; alternatively add a small
transformer function that converts snake_case fields to camelCase before the
existing checks so the rest of the code can rely on accessToken.
…s and fixed RabbitMQ messaging
Summary by CodeRabbit
Release Notes
Refactor
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.