Closed rajdip-b closed 5 days ago
โฑ๏ธ Estimated effort to review [1-5] | 4 |
๐งช Relevant tests | No |
๐ Security concerns |
- Sensitive Information Exposure: As mentioned in the PR description and seen in the code, there is a risk of exposing sensitive information such as secrets in plain text. This needs immediate attention and rectification to ensure that all sensitive data is transmitted securely, preferably using encryption. |
โก Key issues to review |
Possible Security Issue: The PR mentions that secrets will be sent in plain text if the project contains the private key and the user has access to it. This is a significant security risk, especially for a production environment. It is recommended to implement encryption for such sensitive data transmission. |
Code Quality Concern: The use of process.exit(1) within the action method in run.command.ts could lead to abrupt termination of the process without proper cleanup or error handling. Consider implementing a more graceful error handling strategy. | |
Resource Management: In run.command.ts , the child process is spawned and potentially killed and restarted multiple times. Ensure that all resources (e.g., file handles, network connections) are properly managed during these operations to prevent resource leaks. | |
Error Handling: The error handling in the fetchSecrets and fetchVariables methods in the respective controllers could be improved by providing more detailed error information or handling specific error cases more gracefully. |
Category | Suggestion | Score |
Security |
Use secure WebSocket (WSS) to enhance security during socket communication___ **Use HTTPS for socket connection to enhance security, especially if API keys are beingtransmitted.** [apps/cli/src/commands/run/run.command.ts [87-93]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-ad2c4906dc4a080f35a9cb6fa4e4992ea00dd9f5b553e8edea6a21e1bc9062deR87-R93) ```diff -const ioClient = io(`ws://${host}/change-notifier`, { +const ioClient = io(`wss://${host}/change-notifier`, { autoConnect: false, extraHeaders: { 'x-keyshade-token': data.apiKey }, transports: ['websocket'] }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 10Why: Switching to secure WebSocket (WSS) is crucial for enhancing security, especially when transmitting sensitive information like API keys. | 10 |
Replace the insecure
___
**The | 9 | |
Possible issue |
Add error handling to the
___
**Consider adding error handling for the | 9 |
Correct the version number to match the updated specifier for better dependency management___ **It appears that the version specifier for@semantic-release/github has been updated, but the version itself has been incorrectly updated to an older version than what might have been intended. Ensure that the version matches or exceeds the specifier to avoid potential issues with outdated dependencies.** [pnpm-lock.yaml [24-25]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR24-R25) ```diff '@semantic-release/github': specifier: ^10.0.3 - version: 10.0.5(semantic-release@23.1.1(typescript@5.5.2)) + version: 10.0.3(semantic-release@23.1.1(typescript@5.5.2)) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Ensuring the version number matches the specifier is crucial for maintaining dependency consistency and avoiding potential issues with outdated dependencies. | 8 | |
Replace the infinite loop with a controlled loop to manage process lifecycle better___ **Replace the infinite loop inexecuteCommand with a more controlled loop mechanism to prevent potential high CPU usage and improve the manageability of the process lifecycle.** [apps/cli/src/commands/run/run.command.ts [126-157]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-ad2c4906dc4a080f35a9cb6fa4e4992ea00dd9f5b553e8edea6a21e1bc9062deR126-R157) ```diff -while (true) { +let running = true; +while (running) { if (this.shouldRestart) { Logger.info('Restarting command...') process.kill(-childProcess.pid) this.shouldRestart = false } if (childProcess === null) { childProcess = spawn(command, { stdio: ['inherit', 'pipe', 'pipe'], shell: true, env: this.processEnvironmentalVariables, detached: true }) } - await this.sleep(1000) + await this.sleep(1000); + // Include a condition to break the loop if needed + if (someCondition) { + running = false; + } } ``` Suggestion importance[1-10]: 7Why: The suggestion improves the manageability of the process lifecycle and prevents potential high CPU usage, but the exact condition to break the loop is not provided, which leaves some ambiguity. | 7 | |
Ensure Node.js runtime compatibility with the
___
**The | 7 | |
Check compatibility of new
___
**Verify the compatibility of the newly added | 6 | |
Enhancement |
Add error handling for the child process creation to enhance robustness___ **Add error handling for thespawn command to catch and log potential errors that occur when spawning the child process.** [apps/cli/src/commands/run/run.command.ts [135-140]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-ad2c4906dc4a080f35a9cb6fa4e4992ea00dd9f5b553e8edea6a21e1bc9062deR135-R140) ```diff -childProcess = spawn(command, { - stdio: ['inherit', 'pipe', 'pipe'], - shell: true, - env: this.processEnvironmentalVariables, - detached: true -}) +try { + childProcess = spawn(command, { + stdio: ['inherit', 'pipe', 'pipe'], + shell: true, + env: this.processEnvironmentalVariables, + detached: true + }) +} catch (error) { + Logger.error(`Failed to spawn child process: ${error.message}`); + return; +} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Adding error handling for the `spawn` command is a significant improvement that enhances the robustness and reliability of the code. | 9 |
Add input validation for CLI options to ensure correct and non-empty inputs___ **Implement input validation for CLI options to ensure that the inputs meet the expectedformat or constraints before proceeding with configuration. This can prevent potential runtime errors and provide user feedback for correction.** [apps/cli/src/commands/configure/configure.command.ts [63-90]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-d30472a3d78404f1c212c800887469f7c6269efcf8f1c33275a22ef912e3cd93R63-R90) ```diff workspace = await text({ - message: 'Enter the workspace name' + message: 'Enter the workspace name', + validate: input => input.trim() !== '' ? true : 'Workspace name cannot be empty' }) project = await text({ - message: 'Enter the project name' + message: 'Enter the project name', + validate: input => input.trim() !== '' ? true : 'Project name cannot be empty' }) environment = await text({ - message: 'Enter the environment name' + message: 'Enter the environment name', + validate: input => input.trim() !== '' ? true : 'Environment name cannot be empty' }) apiKey = await text({ - message: 'Enter the API key' + message: 'Enter the API key', + validate: input => input.trim() !== '' ? true : 'API key cannot be empty' }) privateKey = await text({ - message: 'Enter the private key' + message: 'Enter the private key', + validate: input => input.trim() !== '' ? true : 'Private key cannot be empty' }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Implementing input validation for CLI options ensures that the inputs meet the expected format or constraints, preventing potential runtime errors and providing immediate user feedback for correction. | 8 | |
Consider replacing
___
**Review the necessity of adding | 7 | |
Verify the necessity of reintroducing the
___
**The | 6 | |
Maintainability |
Add test scripts for new CLI-related tasks to ensure code quality___ **Ensure that the newly added CLI-related scripts in thescripts section of package.json have corresponding test scripts to maintain high code quality and avoid regressions. This is crucial for continuous integration environments and for developers to verify changes locally before pushing.** [package.json [109-114]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R109-R114) ```diff "build:cli": "turbo run build --filter=cli", +"test:cli": "turbo run test --filter=cli", "start:cli": "turbo run start --filter=cli" ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Adding test scripts for new CLI-related tasks is crucial for maintaining high code quality and avoiding regressions, especially in continuous integration environments. | 9 |
Refactor to separate the existing configuration check into a new method___ **Refactor thecreateConfiguration method to separate concerns for better maintainability. Specifically, separate the logic for checking and handling existing configurations into a new method.** [apps/cli/src/commands/configure/configure.command.ts [94-118]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-d30472a3d78404f1c212c800887469f7c6269efcf8f1c33275a22ef912e3cd93R94-R118) ```diff -const userRootConfigExists = existsSync( - getUserRootConfigurationFilePath(project) -) -if (userRootConfigExists) { - const overwrite = await confirm({ - message: 'Overwrite existing user configuration?' - }) - if (isCancel(overwrite)) { - upsertUserRootConfig = false - } -} -const projectRootConfigExists = existsSync('keyshade.json') -if (projectRootConfigExists) { - const overwrite = await confirm({ - message: 'Overwrite existing project configuration?' - }) - if (isCancel(overwrite)) { - upsertProjectRootConfig = false - } -} +await this.handleExistingConfigurations(parsedData) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Refactoring to separate the logic for checking and handling existing configurations improves code maintainability and readability. However, it does not address any critical issues. | 7 | |
Possible bug |
Add checks to ensure the child process exists before attempting to kill it___ **Implement a mechanism to handle the case wherechildProcess.pid is undefined to avoid potential runtime errors when attempting to kill an undefined process.** [apps/cli/src/commands/run/run.command.ts [131]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-ad2c4906dc4a080f35a9cb6fa4e4992ea00dd9f5b553e8edea6a21e1bc9062deR131-R131) ```diff -process.kill(-childProcess.pid) +if (childProcess && childProcess.pid) { + process.kill(-childProcess.pid); +} else { + Logger.error('Attempted to kill a non-existent process.'); +} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: This suggestion addresses a potential runtime error by ensuring that the child process exists before attempting to kill it, improving the code's robustness. | 8 |
Best practice |
Use a structured logger for consistency in logging___ **Replace the direct console logging with a more structured logging approach using adedicated logger. This will help in maintaining consistent logging formats and potentially managing log levels in a centralized way.** [apps/cli/src/commands/configure/configure.command.ts [126-134]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-d30472a3d78404f1c212c800887469f7c6269efcf8f1c33275a22ef912e3cd93R126-R134) ```diff -console.log('writing user root config') -console.log('writing project root config') +Logger.info('writing user root config') +Logger.info('writing project root config') ``` - [x] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Using a structured logger improves consistency in logging and helps in managing log levels centrally, which is a best practice for maintainable and scalable applications. | 8 |
Use exact versions for dependencies to enhance reliability and consistency___ **Consider specifying exact versions for the newly added dependencies to avoid potentialissues with automatic updates that might introduce breaking changes or bugs. Using exact versions helps ensure that all environments run the same code, which can be crucial for debugging and reliability.** [package.json [159-161]](https://github.com/keyshade-xyz/keyshade/pull/289/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R159-R161) ```diff -"zod": "^3.23.6", -"chalk": "^4.1.2", -"moment": "^2.30.1" +"zod": "3.23.6", +"chalk": "4.1.2", +"moment": "2.30.1" ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Using exact versions for dependencies is a best practice that helps ensure consistency across different environments, reducing the risk of unexpected issues due to automatic updates. | 8 |
Attention: Patch coverage is 78.57143%
with 9 lines
in your changes missing coverage. Please review.
Project coverage is 88.04%. Comparing base (
ce50743
) to head (87bc4e2
). Report is 38 commits behind head on develop.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Issues
1 New issue
1 Accepted issue
Measures
0 Security Hotspots
0.0% Coverage on New Code
4.7% Duplication on New Code
User description
Description
The CLI
The main aim of this PR is to add the base keyshade CLI. Right now, the CLI is very fragile (mostly contains experimental code), but contains two of the main commands:
keyshade configure
keyshade run
These commands will allow you to configure any project of yours to be configured and used with keyshade.
Changes in API
We had to make some changes to how the API sent data to the CLI. We added a few endpoints and modified some. One of the things we would like to point out is, secrets would be sent in plain text to the users for now if the project contains the private key and the user has access to it.
Testing the CLI
PORT_NUMBER
apps/cli
and runpnpm watch
node dist/index.js configure
(fromapps/cli
)run
command usingnode dist/index.js run "node test.js"
Notice how the program will start printing the
PORT_NUMBER
value. Try adding a revision, say setting the value to something else. Observe the CLI pick the change and reflect it automatically.PR Type
Enhancement, Documentation
Description
configure
andrun
with real-time updates and configuration management.Changes walkthrough ๐
22 files
run.command.ts
Implement `RunCommand` for executing commands with real-time updates
apps/cli/src/commands/run/run.command.ts
RunCommand
class to handle therun
command.executing commands.
secret.service.ts
Enhance secret service with environment-specific methods and
notifications
apps/api/src/secret/service/secret.service.ts
flag.
variable.service.ts
Enhance variable service with environment-specific methods and
notifications
apps/api/src/variable/service/variable.service.ts
plaintext flag.
configure.command.ts
Implement `ConfigureCommand` for CLI configuration management
apps/cli/src/commands/configure/configure.command.ts
ConfigureCommand
class to handle theconfigure
command.configuration.ts
Add configuration utility functions for CLI
apps/cli/src/util/configuration.ts
configurations.
variable.controller.ts
Add endpoint for fetching all variables of a project and environment
apps/api/src/variable/controller/variable.controller.ts
updateVariable
method to useUpdateVariable
DTO.api-key.service.ts
Enhance API key update logic with authority merging
apps/api/src/api-key/service/api-key.service.ts
updateApiKey
method to merge existing and new authorities.logger.ts
Add Logger utility for formatted logging
apps/cli/src/util/logger.ts
Logger
utility with methods for logging info, error, andwarning messages.
chalk
andmoment
for formatted logging.secret.ts
Add SecretController for fetching secrets via HTTP
apps/cli/src/http/secret.ts
SecretController
for fetching secrets via HTTP.variable.ts
Add VariableController for fetching variables via HTTP
apps/cli/src/http/variable.ts
VariableController
for fetching variables via HTTP.change-notifier.socket.ts
Enhance client registration with detailed response and logging
apps/api/src/socket/change-notifier.socket.ts
registration.
index.ts
Set up CLI entry point with commander
apps/cli/src/index.ts
commander
.ConfigureCommand
andRunCommand
.auth.guard.ts
Enhance AuthGuard error handling for API key validation
apps/api/src/auth/guard/auth/auth.guard.ts
AuthGuard
for API key validation.auth.ts
Add AuthController for API key validation
apps/cli/src/http/auth.ts
AuthController
for checking API key validity via HTTP.secret.controller.ts
Add endpoint for fetching all secrets of a project and environment
apps/api/src/secret/controller/secret.controller.ts - Added endpoint to fetch all secrets for a project and environment.
api-key.controller.ts
Add endpoint to check API key access for live updates
apps/api/src/api-key/controller/api-key.controller.ts - Added endpoint to check if API key can access live updates.
socket.types.ts
Update socket types with client registration response and plaintext
flag
apps/api/src/socket/socket.types.ts
ClientRegisteredResponse
interface.isSecret
toisPlaintext
inChangeNotification
interface.run.types.d.ts
Add TypeScript definitions for run command types
apps/cli/src/commands/run/run.types.d.ts
Configuration
andClientRegisteredResponse
.configure.types.d.ts
Add TypeScript definitions for configure command types
apps/cli/src/commands/configure/configure.types.d.ts
ProjectRootConfig
andUserRootConfig
.index.ts
Export HTTP controllers for CLI
apps/cli/src/http/index.ts
SecretController
,VariableController
, andAuthController
.command.interface.ts
Define BaseCommand interface for CLI commands
apps/cli/src/commands/base/command.interface.ts - Defined `BaseCommand` interface for CLI commands.
constants.ts
Add API base URL constant
apps/cli/src/util/constants.ts - Added `API_BASE_URL` constant.
5 files
.eslintrc.js
Add ESLint configuration for CLI
apps/cli/.eslintrc.js - Added ESLint configuration for the CLI project.
package.json
Update package.json with CLI scripts and dependencies
package.json
package.json
Create package.json for CLI project
apps/cli/package.json
tsconfig.json
Add TypeScript configuration for CLI
apps/cli/tsconfig.json - Added TypeScript configuration for the CLI project.
docker-compose.yml
Add volumes to docker-compose for persistent storage
docker-compose.yml - Added volumes for database, Redis, and MinIO services.
1 files
pnpm-lock.yaml
Update dependencies and add new ones for CLI
pnpm-lock.yaml
commander
,socket.io-client
, andtypescript
.