Closed anudeeps352 closed 3 weeks ago
Here are some key observations to aid the review process:
**🎫 Ticket compliance analysis 🔶** **[302](https://github.com/keyshade-xyz/keyshade/issues/302) - Partially compliant** Fully compliant requirements: - List all secrets under a project Not compliant requirements: - Fetch all revisions of a secret - Create a secret - Update a secret - Rollback a secret - Delete a secret - Create an implementation of `command.interface.ts` and name it `SecretCommand` - Stash all the functions in `src/commands/secret` - Use the `secretController` under `ControllerInstance` to make the API calls |
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪ |
🧪 No relevant tests |
🔒 No security concerns identified |
⚡ Recommended focus areas for review Code Cleanup Verify if removing the console.log statement affects any debugging or logging functionality |
Latest suggestions up to 96e99d6 Explore these optional code suggestions:
Category | Suggestion | Score |
Enhancement |
Provide user feedback when no items are found in a list operation___ **Consider handling the case wheresecrets.length is 0 to provide feedback to the user when no secrets are found.** [apps/cli/src/commands/secret/list.secret.ts [44-46]](https://github.com/keyshade-xyz/keyshade/pull/515/files#diff-2a60ffa339c54c754c130fc45b4cda2ef7dd8cc472e8e18c2c329f8f3031ae7bR44-R46) ```diff if (secrets.length > 0) { data.forEach((secret: any) => { Logger.info(`- ${secret.name} (${secret.value})`) }) +} else { + Logger.info('No secrets found.') } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: The suggestion improves user experience by providing feedback when no secrets are found, which is a valuable enhancement for understanding the command's output. | 8 |
Best practice |
Improve type safety by using a specific interface instead of 'any' for loop variables___ **Consider using a more specific type for thesecret parameter in the forEach loop instead of any . This will provide better type safety and code clarity.**
[apps/cli/src/commands/secret/list.secret.ts [45-46]](https://github.com/keyshade-xyz/keyshade/pull/515/files#diff-2a60ffa339c54c754c130fc45b4cda2ef7dd8cc472e8e18c2c329f8f3031ae7bR45-R46)
```diff
-data.forEach((secret: any) => {
+interface Secret {
+ name: string;
+ value: string;
+}
+
+data.forEach((secret: Secret) => {
Logger.info(`- ${secret.name} (${secret.value})`)
})
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 7Why: The suggestion to use a specific interface instead of 'any' enhances type safety and code clarity, which is beneficial for maintainability and reducing potential runtime errors. | 7 |
💡 Need additional feedback ? start a PR chat
Category | Suggestion | Score |
Security |
Remove unnecessary console logging of potentially sensitive data___ **Remove the console.log statement as it's unnecessary and may expose sensitiveinformation. The Logger is already being used to display the necessary information.** [apps/cli/src/commands/secret/list.secret.ts [43-54]](https://github.com/keyshade-xyz/keyshade/pull/515/files#diff-2a60ffa339c54c754c130fc45b4cda2ef7dd8cc472e8e18c2c329f8f3031ae7bR43-R54) ```diff if (success) { - console.log(data) const secrets = data if (secrets.length > 0) { data.items.forEach((secret: any) => { Logger.info(`- ${secret.name} (${secret.value})`) }) } else { Logger.info('No secrets found') } } else { Logger.error(`Failed fetching secrets: ${error.message}`) } ``` Suggestion importance[1-10]: 9Why: Removing the console.log statement is crucial as it may expose sensitive information. The Logger is already being used to display necessary information, making the console.log redundant and potentially harmful. | 9 |
Possible issue |
Validate the rotateAfter option to ensure it only accepts allowed values___ **Add input validation for therotateAfter option to ensure it only accepts valid values ('24', '168', '720', '8760', or 'never').** [apps/cli/src/commands/secret/create.secret.ts [214-256]](https://github.com/keyshade-xyz/keyshade/pull/515/files#diff-bc7d8acc049f3b4f6ecf35eb5e4422a3da650ed274ef906f2123b5b7f0849e6dR214-R256) ```diff private async parseInput(options: CommandActionData['options']): Promise<{ name: string note?: string rotateAfter?: '24' | '168' | '720' | '8760' | 'never' entries: Array<{ value: string; environmentSlug: string }> }> { let { name, note, rotateAfter } = options const { entries } = options if (!name) { name = await text({ message: 'Enter the name of secret', placeholder: 'My Secret' }) } if (!entries) { throw new Error('Entries is required') } if (!note) { note = name } + if (rotateAfter && !['24', '168', '720', '8760', 'never'].includes(rotateAfter)) { + throw new Error('Invalid rotateAfter value. Must be 24, 168, 720, 8760, or never.') + } + const parsedEntries = entries.map((entry) => { const entryObj: { value: string; environmentSlug: string } = { value: '', environmentSlug: '' } entry.split(' ').forEach((pair) => { const [key, value] = pair.split('=') entryObj[key] = value }) return entryObj }) return { name, note, rotateAfter, entries: parsedEntries } } ``` Suggestion importance[1-10]: 8Why: Adding validation for the rotateAfter option is important to prevent invalid values from being processed, which could lead to unexpected behavior or errors. This enhances the robustness of the input handling. | 8 |
Validate required options before making the API call for secret rollback___ **Add validation for the requiredenvironmentSlug and version options to ensure they are provided before making the API call.** [apps/cli/src/commands/secret/rollback.secret.ts [122-144]](https://github.com/keyshade-xyz/keyshade/pull/515/files#diff-243a4813cca8910af061f7043d4737ceedb1191457366da26ad1d930d4e853feR122-R144) ```diff async action({ args, options }: CommandActionData): Promise Suggestion importance[1-10]: 8Why: Validating the required options before making the API call is essential to prevent unnecessary API requests and to provide immediate feedback to the user, enhancing the user experience and reliability of the command. | 8 | |
Enhancement |
Parse and validate the entries option before updating the secret___ **Parse and validate theentries option before passing it to the controller to ensure proper formatting and prevent potential errors.** [apps/cli/src/commands/secret/update.secret.ts [131-152]](https://github.com/keyshade-xyz/keyshade/pull/515/files#diff-9a4b1a14b890ec20b42863918a14c94766227559fc9ef3986d3d5c54cedf90ceR131-R152) ```diff async action({ args, options }: CommandActionData): Promise Suggestion importance[1-10]: 7Why: Parsing and validating the entries option ensures that the data is correctly formatted before being sent to the controller, reducing the risk of errors and improving data integrity. | 7 |
I think somehow you have comitted all the files again.
Yeah I was also thinking the same. I dunno how it happened,I only changed the console log
Hey if its easy enough then ig you can directly modify the file itself and just close this pr right ? I dont think such a small issue requires a pr
Ig now its fine right ?
:tada: This PR is included in version 2.7.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
User description
Description
Removed an unnecessary console log
Fixes #302
Dependencies
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
enhancement
Description
ListSecret
command in the CLI application.PRDescriptionHeader.CHANGES_WALKTHROUGH
list.secret.ts
Remove unnecessary console log from ListSecret command
apps/cli/src/commands/secret/list.secret.ts