feat: add encryption for api keys #331
Conversation
WalkthroughThe changes in this pull request enhance the API key management system by modifying the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (11)
apps/api/src/database/migrations/1727156770079-AddMaskedKeyColumn.ts (2)
4-14: LGTM: 'up' method correctly adds the new column.The method properly adds a 'masked_api_key' column to the 'notify_server_api_keys' table. The column definition is appropriate for storing a masked API key.
Consider adding a comment explaining the purpose of the 'masked_api_key' column for better code documentation. For example:
// Add 'masked_api_key' column to store partially obscured API keys for display purposes await queryRunner.addColumn( // ... (rest of the code remains the same) );
1-18: LGTM: Migration successfully implements the required changes for API key encryption.This migration file correctly adds a 'masked_api_key' column to the 'notify_server_api_keys' table, which aligns with the PR objective of implementing encryption for API keys. The migration is well-structured, follows TypeORM conventions, and is reversible.
As this migration is part of a larger feature to encrypt API keys, ensure that:
- The actual encryption of API keys is implemented in the application logic.
- The 'masked_api_key' column is populated with partially visible versions of the encrypted keys.
- Any code that previously accessed the plain API keys is updated to use the encrypted versions.
- The API documentation is updated to reflect these changes in key handling.
apps/api/src/modules/server-api-keys/server-api-keys.resolver.ts (2)
7-10: LGTM: Class definition is well-structured and secure.The
ServerApiKeysResolverclass is correctly decorated and follows best practices:
@Resolver(() => ServerApiKey)properly associates the resolver with theServerApiKeyentity.@UseGuards(GqlAuthGuard)ensures that all resolver methods are protected by authentication.- Constructor injection of
ServerApiKeysServicefollows dependency injection principles.Consider adding error handling for cases where
ServerApiKeysServiceinitialization fails. For example:constructor(private serverApiKeysService: ServerApiKeysService) { if (!serverApiKeysService) { throw new Error('ServerApiKeysService is not initialized'); } }
12-17: LGTM: Mutation method is well-defined, but could benefit from additional safeguards.The
generateApiKeymutation is correctly implemented:
- It's properly decorated with
@Mutation(() => String).- The
applicationIdargument is correctly typed and decorated.- The method correctly delegates to the
ServerApiKeysService.Consider adding input validation and error handling:
- Validate
applicationId:if (applicationId <= 0) { throw new Error('Invalid applicationId'); }
- Add try-catch for error handling:
@Mutation(() => String) async generateApiKey( @Args('applicationId', { type: () => Int }) applicationId: number, ): Promise<string> { if (applicationId <= 0) { throw new Error('Invalid applicationId'); } try { return await this.serverApiKeysService.generateApiKey(applicationId); } catch (error) { // Log the error console.error('Failed to generate API key:', error); throw new Error('Failed to generate API key'); } }These additions will improve the robustness of the mutation.
apps/api/src/common/utils/bcrypt.ts (3)
11-12: Approved: Good use of configurable SALT_ROUNDS.The change to use SALT_ROUNDS from configuration improves flexibility. However, consider making this function asynchronous for better performance in high-load scenarios.
Here's a suggested asynchronous version:
export async function encodePassword(rawPassword: string): Promise<string> { const salt = await bcrypt.genSalt(this.SALT_ROUNDS); return bcrypt.hash(rawPassword, salt); }This change would make the function consistent with the newly added asynchronous API key functions.
19-25: Approved: Well-implemented API key encryption function.The new
encryptApiKeyfunction is well-implemented, using async/await for better performance and combining the API key with a SECRET for enhanced security. The use of configurable SALT_ROUNDS is consistent with the password encoding function.Consider adding error handling to make the function more robust:
export const encryptApiKey = async (originalApiKey: string): Promise<string> => { try { const keyToHash = `${originalApiKey}${this.SECRET}`; const salt = await bcrypt.genSalt(this.SALT_ROUNDS); const encryptedApiKey = await bcrypt.hash(keyToHash, salt); return encryptedApiKey; } catch (error) { // Log the error or handle it as appropriate for your application throw new Error('Failed to encrypt API key'); } };This change would help catch and handle any unexpected errors during the encryption process.
27-35: Approved: Well-implemented API key comparison function.The new
compareApiKeysfunction is well-implemented, using async/await for better performance and combining the API key with the SECRET before comparison, which is consistent with the encryption function. The use of bcrypt.compare is the correct approach for comparing hashed values.Consider adding error handling to make the function more robust:
export const compareApiKeys = async ( originalApiKey: string, encryptedApiKey: string, ): Promise<boolean> => { try { const keyToCompare = `${originalApiKey}${this.SECRET}`; const isMatch = await bcrypt.compare(keyToCompare, encryptedApiKey); return isMatch; } catch (error) { // Log the error or handle it as appropriate for your application throw new Error('Failed to compare API keys'); } };This change would help catch and handle any unexpected errors during the comparison process.
apps/api/src/modules/server-api-keys/server-api-keys.service.ts (3)
8-9: Remove unnecessarybcryptimport after replacing API key generation.Since you're replacing
bcrypt.genSalt()withcrypto.randomBytes()for API key generation, the import ofbcryptis no longer needed. Removing unused imports helps keep the codebase clean.Apply this diff to remove the unused import:
-import * as bcrypt from 'bcrypt';
36-36: Review the masking strategy to prevent information leakage.Revealing the first and last 4 characters of the API key may expose too much information, potentially aiding attackers in compromising the key. Consider reducing the number of exposed characters or modifying the masking pattern.
You could modify the masking to reveal fewer characters:
- const maskedApiKey = `${originalApiKey.slice(0, 4)}****${originalApiKey.slice(-4)}`; + const maskedApiKey = `${originalApiKey.slice(0, 2)}****${originalApiKey.slice(-2)}`;
44-44: Add error handling when saving the API key to the repository.If
save(serverApiKey)fails due to a database error or other issue, the error will propagate unhandled. Consider adding error handling to manage potential exceptions and provide meaningful feedback.Example of adding error handling:
try { await this.serverApiKeyRepository.save(serverApiKey); } catch (error) { // Handle the error appropriately, e.g., log the error and throw a custom exception }apps/api/src/common/guards/api-key/api-key.guard.ts (1)
98-109: Potential security risk if API keys are not handled properlyStoring and handling API keys requires careful attention to security best practices to prevent leaks or unauthorized access.
Ensure that:
- API keys are stored securely, preferably hashed and salted.
- Access to API keys is restricted and audited.
- Logs do not contain sensitive information.
Review the overall API key management strategy to align with security standards.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- apps/api/src/common/guards/api-key/api-key.guard.ts (2 hunks)
- apps/api/src/common/utils/bcrypt.ts (1 hunks)
- apps/api/src/database/migrations/1727156770079-AddMaskedKeyColumn.ts (1 hunks)
- apps/api/src/modules/server-api-keys/entities/server-api-key.entity.ts (1 hunks)
- apps/api/src/modules/server-api-keys/server-api-keys.module.ts (1 hunks)
- apps/api/src/modules/server-api-keys/server-api-keys.resolver.ts (1 hunks)
- apps/api/src/modules/server-api-keys/server-api-keys.service.ts (2 hunks)
Additional comments not posted (9)
apps/api/src/modules/server-api-keys/server-api-keys.module.ts (2)
5-5: LGTM! Changes align with NestJS best practices.The addition of
ServerApiKeysResolverto both the imports and providers is correct. This change makes the resolver available for dependency injection within the module, which is necessary for handling GraphQL operations related to server API keys.Also applies to: 9-9
5-5: Verify encryption implementation for API keys.While these changes are correct, they don't directly address the PR's main objective of implementing encryption for API keys. Could you please clarify where the encryption logic is implemented? Is it within the
ServerApiKeysResolver, or is it handled elsewhere in the codebase?To help verify the implementation, you can run the following script:
This script will help us locate where the encryption logic is implemented within the server-api-keys module.
Also applies to: 9-9
Verification successful
Encryption for API keys is correctly implemented within the
ServerApiKeysService.
- The
encryptApiKeyfunction fromsrc/common/utils/bcryptis used to encrypt API keys before storage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for encryption-related code in the ServerApiKeysResolver and related files # Check ServerApiKeysResolver for encryption logic echo "Checking ServerApiKeysResolver for encryption logic:" rg --type typescript -i "encrypt|cipher|crypto" apps/api/src/modules/server-api-keys/server-api-keys.resolver.ts # Check ServerApiKeysService for encryption logic echo "Checking ServerApiKeysService for encryption logic:" rg --type typescript -i "encrypt|cipher|crypto" apps/api/src/modules/server-api-keys/server-api-keys.service.ts # Check for any new files related to encryption in the server-api-keys module echo "Checking for new encryption-related files:" fd -e ts -d 1 encrypt apps/api/src/modules/server-api-keysLength of output: 682
Script:
#!/bin/bash # Description: Search for encryption-related code in the ServerApiKeysResolver and related files without specifying file type # Check ServerApiKeysResolver for encryption logic echo "Checking ServerApiKeysResolver for encryption logic:" rg -i "encrypt|cipher|crypto" apps/api/src/modules/server-api-keys/server-api-keys.resolver.ts # Check ServerApiKeysService for encryption logic echo "Checking ServerApiKeysService for encryption logic:" rg -i "encrypt|cipher|crypto" apps/api/src/modules/server-api-keys/server-api-keys.service.ts # Check for any encryption-related code within all .ts files in the server-api-keys module echo "Searching all TypeScript files in server-api-keys for encryption logic:" rg -i "encrypt|cipher|crypto" apps/api/src/modules/server-api-keys/**/*.tsLength of output: 829
apps/api/src/database/migrations/1727156770079-AddMaskedKeyColumn.ts (3)
1-1: LGTM: Imports are correct and necessary.The imports from 'typeorm' are appropriate for creating a database migration using TypeORM.
3-3: LGTM: Class definition follows TypeORM migration conventions.The class name
AddMaskedKeyColumn1727156770079includes a timestamp and implementsMigrationInterface, which is the correct approach for TypeORM migrations.
15-17: LGTM: 'down' method correctly reverts the migration.The method properly drops the 'masked_api_key' column from the 'notify_server_api_keys' table, effectively reverting the changes made in the 'up' method.
apps/api/src/modules/server-api-keys/server-api-keys.resolver.ts (1)
1-5: LGTM: Imports are appropriate and aligned with the resolver's functionality.The imports cover all necessary decorators, guards, and services required for implementing the API key generation resolver. They align well with the PR objective of adding encryption for API keys.
apps/api/src/common/utils/bcrypt.ts (1)
1-35: Overall: Good implementation of API key encryption, aligning with PR objectives.The changes in this file successfully implement the encryption and comparison of API keys, which aligns well with the PR objective of "feat: add encryption for api keys". The use of ConfigService for retrieving configuration values enhances flexibility and security. The new functions for API key handling are well-implemented and consistent with existing patterns.
Some suggestions for further improvement:
- Consider injecting ConfigService instead of instantiating it directly.
- Add error handling to the new API key functions.
- Consider making the
encodePasswordfunction asynchronous for consistency and potential performance benefits.These changes have significantly enhanced the security of API key handling in the application.
apps/api/src/modules/server-api-keys/entities/server-api-key.entity.ts (1)
26-28: Approve addition ofmaskedApiKeyproperty with suggestions for improvementThe addition of the
maskedApiKeyproperty aligns well with the PR objective of implementing encryption for API keys. The property is correctly annotated for both database storage and GraphQL schema.However, I have a few suggestions and questions:
Could you clarify the encryption process and the relationship between
apiKeyandmaskedApiKey? It would be helpful to understand how these two properties interact.Consider adding validation for the
maskedApiKeyformat. For example, you might want to ensure it follows a specific pattern or length.Remember to update any related documentation to reflect this new property.
As mentioned in the PR objectives, ensure that the
.env.examplefile is updated if any new environment variables are required for this feature.To verify the implementation of the masking process, please run the following script:
apps/api/src/modules/server-api-keys/server-api-keys.service.ts (1)
24-31: Ensure all usages offindByRelatedApplicationIdhandle the array return type.The method
findByRelatedApplicationIdnow returns aPromise<ServerApiKey[]>instead ofPromise<ServerApiKey | undefined>. Verify that all callers of this method are updated to handle an array ofServerApiKeyobjects to prevent runtime errors.Run the following script to find all usages of
findByRelatedApplicationIdand review how the return value is handled:Verification successful
All usages of
findByRelatedApplicationIdhandle the array return type appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `findByRelatedApplicationId` to ensure callers handle the array return type. # Search for usages of the method in TypeScript files. rg --type ts 'findByRelatedApplicationId\(' -A 5Length of output: 1183
apps/api/src/modules/server-api-keys/server-api-keys.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/modules/server-api-keys/server-api-keys.service.ts
Outdated
Show resolved
Hide resolved
API PR Checklist
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
This Pr does the following things
Related changes:
Screenshots:
// api
//db
Query request and response:
Documentation changes:
NA
Test suite/unit testing output:
NA
Pending actions:
NA
Additional notes:
NA
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates aim to improve security, flexibility, and user experience within the application.