feat: generate key one time #992
Conversation
|
Warning Rate limit exceeded@elibosley has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
WalkthroughThis pull request introduces modifications to the API key management system, focusing on the addition of local API keys and the restructuring of role permissions. Key changes include the addition of Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 comments (2)
api/src/unraid-api/auth/api-key.service.ts (1)
Line range hint
186-216: Efficient file reads & concurrency concerns.
“findByField” reads all files in a loop and parses them. If more keys accumulate, this approach may degrade performance. Consider indexing or caching for frequent lookups. Also ensure concurrency is handled if multiple writes happen simultaneously.api/dev/states/myservers.cfg (1)
Review and update CORS configuration for security
The investigation reveals that these domains serve specific purposes:
https://studio.apollographql.comis justified due to Apollo GraphQL dependencies in the projectgoogle.comappears in NTP server configuration and as an extra origintest.comappears only as an extra origin and seems unnecessaryRecommendations:
- Remove
https://test.comas it appears to be a testing artifact- Consider restricting Apollo Studio access to development environments only
- Review if
google.comis needed inextraOriginsas it's only functionally required for NTP🔗 Analysis chain
Line range hint
20-20: Review CORS configuration for securityThe
allowedOriginslist includes some potentially broad patterns. Please review if all these origins are necessary, especially:
https://google.comhttps://test.comhttps://studio.apollographql.com🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other files that might reference these origins rg -l "google\.com|test\.com|apollographql\.com" --type-add 'config:*.{cfg,json,yaml,yml}' -t configLength of output: 209
Script:
#!/bin/bash # Let's check the content of these files to understand the context of these domains echo "=== Content of api/dev/Unraid.net/myservers.cfg ===" cat api/dev/Unraid.net/myservers.cfg echo -e "\n=== Content of api/dev/states/var.ini ===" cat api/dev/states/var.ini echo -e "\n=== Checking package.json for Apollo related dependencies ===" cat api/package.json | grep -A 5 -B 5 "apollo"Length of output: 5153
🧰 Tools
🪛 Gitleaks (8.21.2)
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🧹 Nitpick comments (5)
api/src/unraid-api/auth/api-key.service.ts (1)
88-90: Check logic for overwritten API keys.
If an API key already exists and we do not set 'overwrite' to true, we throw an error. Consider providing a more descriptive message or a distinct error code to help callers distinguish this situation from others.api/src/mothership/utils/create-local-connect-api-key.ts (2)
7-7: Add return type annotation for better type safetyThe function should explicitly declare its return type for better type safety and documentation.
-export const createLocalApiKeyForConnectIfNecessary = async () => { +export const createLocalApiKeyForConnectIfNecessary = async (): Promise<void> => {
16-16: Potential race condition in key existence checkThe check for existing key and creation of new key is not atomic, which could lead to race conditions in concurrent scenarios.
Consider implementing a locking mechanism or using a transaction-like pattern to ensure atomic operations when creating new keys.
api/src/unraid-api/graph/connect/connect.resolver.ts (1)
Line range hint
54-89: Consider adding rate limiting for remote access operationsThe remote access control logic could benefit from rate limiting to prevent potential abuse of the API.
Consider implementing rate limiting middleware for these sensitive operations to enhance security.
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json (1)
1-10: Separate development and production configurations.This API key file is located in the
devdirectory but appears to contain production-like configurations. Consider:
- Using environment-specific configuration patterns
- Implementing a proper secret management solution
- Documenting the key generation process for different environments
Would you like help designing a more robust configuration management approach?
🧰 Tools
🪛 Gitleaks (8.21.2)
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (14)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json(1 hunks)api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json(1 hunks)api/dev/states/myservers.cfg(1 hunks)api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts(2 hunks)api/src/graphql/schema/types/auth/auth.graphql(0 hunks)api/src/graphql/schema/types/auth/roles.graphql(1 hunks)api/src/index.ts(2 hunks)api/src/mothership/utils/create-local-connect-api-key.ts(1 hunks)api/src/store/listeners/listener-middleware.ts(2 hunks)api/src/store/listeners/local-api-key-listener.ts(0 hunks)api/src/unraid-api/auth/api-key.service.ts(8 hunks)api/src/unraid-api/auth/casbin/policy.ts(1 hunks)api/src/unraid-api/graph/connect/connect.resolver.ts(3 hunks)
💤 Files with no reviewable changes (2)
- api/src/store/listeners/local-api-key-listener.ts
- api/src/graphql/schema/types/auth/auth.graphql
🧰 Additional context used
🪛 Gitleaks (8.21.2)
api/dev/Unraid.net/myservers.cfg
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
api/dev/states/myservers.cfg
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (16)
api/src/unraid-api/auth/api-key.service.ts (4)
28-28: Consider security implications of in-memory storage.
Storing API keys and secrets in memory can be convenient, but be mindful of potential exposure through process dumps or logs. Evaluate if additional encryption or other protective measures are necessary to mitigate risks.
55-59: Validate international characters and expand usage messages.
The current regex restricts the name to alphanumeric plus spaces. If you expect usage beyond basic ASCII letters (e.g., diacritics), confirm this passes all scenarios or adjust accordingly. Consider clarifying the error message to reflect all valid characters explicitly.
65-66: New 'overwrite' parameter is a beneficial addition.
This parameter clarifies intent when reusing API key names. Ensure that references to this method elsewhere in the codebase handle the new argument correctly, especially in public or library usage contexts.
281-288: Sorted API key structure enhances consistency.
Sorting JSON fields ensures predictable output and simplifies diffs. This change is approved and should also be reflected in any associated backup or replication logic to ensure consistent results.
api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json (1)
6-6: Role updated to 'connect'.
Switching from “upc” to “connect” aligns with the new role-based approach. Validate that the “connect” role is recognized throughout the system to avoid unexpected permission failures.
api/src/unraid-api/auth/casbin/policy.ts (2)
9-11: New 'Connect' permissions
Granting READ_ANY on all resources is broad. Confirm that users with the CONNECT role genuinely need read access to every resource. If more granular control is desired, consider enumerating resources individually.
18-18: Role inheritance from 'Guest'
Inheriting from GUEST ensures baseline privileges remain intact. This is a logical move, but be mindful of any future expansions to GUEST privileges that could inadvertently escalate the CONNECT role.
api/src/graphql/schema/types/auth/roles.graphql (1)
1-42: Resource & Role enums introduced
Defining these enums clarifies available resources and roles in a single place, improving schema maintainability. Ensure references in resolvers, services, or policy definitions stay updated if new roles or resources are added.
api/src/store/listeners/listener-middleware.ts (2)
11-11: LGTM: Notification path listener integration
The addition of enableNotificationPathListener and its integration into startMiddlewareListeners looks correct.
Also applies to: 34-41
Line range hint 1-11: Verify complete migration of local API key generation
The removal of enableLocalApiKeyListener implies a shift in architecture for API key management. Let's verify that all previous functionality is covered by the new implementation.
✅ Verification successful
Migration to new API key generation mechanism verified
The migration appears complete and well-implemented:
- The old listener has been completely removed with no lingering references
- The new implementation in
create-local-connect-api-key.tsproperly handles the API key generation functionality:- Checks config file load status
- Verifies existing key validity
- Creates new key when necessary
- Updates user config with the new key
- Includes proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old listener
# and verify the new implementation's presence
# Check for any remaining references to local-api-key-listener
rg -l "local-api-key-listener"
# Verify the new implementation exists
fd -t f "create-local-connect-api-key.ts"
Length of output: 129
Script:
#!/bin/bash
# Let's verify the completeness of the migration by:
# 1. Check the implementation of the new mechanism
# 2. Search for any lingering references to the old listener pattern
# Check the new implementation
cat api/src/mothership/utils/create-local-connect-api-key.ts
# Search for any references to local API key generation in listener files
rg -l "localApiKey|LocalApiKey" "api/src/store/listeners/"
# Search for the old listener implementation to understand what functionality needs to be covered
fd -t f "local-api-key-listener.ts" || echo "No old listener file found"
Length of output: 1549
api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts (2)
Line range hint 48-51: LGTM: Proper error handling for key creation
The error handling for failed key creation is appropriate and maintains security by preventing progression without a valid key.
46-46: Verify security implications of new key creation method
The new createLocalConnectApiKey method simplifies the API key creation process. Let's verify its implementation to ensure it maintains proper security controls.
✅ Verification successful
Implementation maintains proper security controls
The createLocalConnectApiKey implementation is secure based on the following findings:
- It uses the base
createmethod which includes:- Input validation for required name
- Role validation
- Name sanitization
- It's configured with ADMIN role access level
- The generated key follows the
ApiKeyWithSecrettype which includes proper ID, key, and role constraints - The implementation is consistent with the authentication architecture
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of createLocalConnectApiKey
# Find and examine the implementation
ast-grep --pattern 'createLocalConnectApiKey() {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Let's try different approaches to find the implementation
# Search for the method name in various formats
rg "createLocalConnectApiKey" -A 5
# Also search for the class/service definition
rg "class.*ApiKeyService" -A 10
# Look for files that might contain the implementation
fd "api-key" --type f
Length of output: 3429
Script:
#!/bin/bash
# Let's examine the create method since createLocalConnectApiKey uses it
ast-grep --pattern 'create($$$) {
$$$
}'
# Also check the Role enum to understand the security implications
rg "enum Role" -A 10
# Let's also look at the ApiKeyWithSecret type
rg "type ApiKeyWithSecret" -A 5
Length of output: 3692
Script:
#!/bin/bash
# Let's examine the create method implementation in the ApiKeyService
rg "async create\(" -B 2 -A 15 api/src/unraid-api/auth/api-key.service.ts
# Also check if there are any specific security measures for local API keys
rg "local.*api.*key" -i api/src/unraid-api/auth/api-key.service.ts
Length of output: 721
api/src/unraid-api/graph/connect/connect.resolver.ts (2)
11-11: LGTM: Enhanced type safety with Resource enum
The migration from string literals to the Resource enum improves type safety and maintainability.
Also applies to: 23-23
49-49: Verify permission structure for remote access
Let's ensure the new Resource.CONNECT__REMOTE_ACCESS permission is properly defined and granted to appropriate roles.
✅ Verification successful
Let me gather more information about the role definitions and inheritance to ensure the permission structure is complete.
Permission structure for remote access is properly configured
The permission structure for Resource.CONNECT__REMOTE_ACCESS is correctly defined and granted appropriately:
- The resource is properly defined in the enum as
CONNECT__REMOTE_ACCESS - The
connectrole has explicit permission to performUPDATE_ANYon this resource - The role inheritance is properly set up with
connectrole inheriting fromguest - The permission is being used correctly in the resolver with proper resource and possession attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check permission definitions and usage
# Find permission definitions
rg -A 5 "CONNECT__REMOTE_ACCESS"
# Check policy definitions
fd -t f "policy.ts" | xargs rg "CONNECT__REMOTE_ACCESS"
Length of output: 1576
Script:
#!/bin/bash
# Check role definitions and inheritance
rg -A 3 "enum Role"
# Check if CONNECT role is properly defined and inherited
rg "Role.CONNECT"
# Check for any other related connect permissions
rg "CONNECT__" --type ts
Length of output: 1352
api/src/index.ts (2)
35-35: LGTM: Clean import of the new utility function.
The import is properly placed with other application utilities.
88-89: Verify idempotency of API key creation.
The function is called on every application startup. While the name suggests it only creates the key if necessary, let's verify this behavior to ensure we're not generating unnecessary database operations on each startup.
✅ Verification successful
Let me gather more information about the implementation by checking the complete function and any related database operations.
Based on the implementation in api/src/mothership/utils/create-local-connect-api-key.ts, I can now generate the final response:
API key creation is properly idempotent
The function has proper checks to prevent unnecessary database operations:
- It only creates a new key if
remote.apiKeyexists AND either:remote.localApiKeyis not set, or- The existing
localApiKeyis not found in the database (viaservice.findById)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation to verify idempotency checks
ast-grep --pattern 'function createLocalApiKeyForConnectIfNecessary() {
$$$
}'
# Look for any existing key checks before creation
rg -A 5 "createLocalApiKeyForConnectIfNecessary"
Length of output: 1382
Script:
#!/bin/bash
# Get the complete implementation of createLocalApiKeyForConnectIfNecessary
cat api/src/mothership/utils/create-local-connect-api-key.ts
# Look for any API key related database operations
rg "ApiKey" -A 5
Length of output: 95894
api/dev/states/myservers.cfg
Outdated
| wanport="8443" | ||
| upnpEnabled="no" | ||
| apikey="_______________________BIG_API_KEY_HERE_________________________" | ||
| localApiKey="3a4e2332891e879d2ac8c3f25ef03a7b54f70b62cd6c5a08a86189cdd19ba203" |
There was a problem hiding this comment.
Security concern: Another API key exposed in configuration
Similar to the previous configuration file, this file contains an exposed API key. This compounds the security risk, especially if these files are tracked in version control.
Please apply the same recommendations as before:
- Remove the actual API key
- Add to
.gitignore - Use environment variables or secrets management
- Create a template file
-localApiKey="3a4e2332891e879d2ac8c3f25ef03a7b54f70b62cd6c5a08a86189cdd19ba203"
+localApiKey="YOUR_LOCAL_API_KEY_HERE"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.21.2)
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json (2)
1-10:⚠️ Potential issueCritical: Remove sensitive API key from source control.
API keys should never be committed to source control as they provide access to sensitive operations.
Please:
- Remove this file from source control
- Add
*.jsonpattern to.gitignoreunder theapi/dev/keys/directory- Update documentation to explain the proper way to generate and manage these keys
- Rotate this key as it has been exposed
🧰 Tools
🪛 Gitleaks (8.21.2)
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
7-9:⚠️ Potential issueSecurity: Apply principle of least privilege.
The API key is granted admin role which violates the principle of least privilege. For a Connect user, consider creating a specific role with only the necessary permissions.
api/src/mothership/utils/create-local-connect-api-key.ts (2)
14-14: 🛠️ Refactor suggestionAvoid creating multiple ApiKeyService instances.
Two instances of
ApiKeyServiceare created unnecessarily. Reuse the existing instance for better resource management.const service = new ApiKeyService(); if (remote.apikey && (!remote.localApiKey || !(await service.findByKey(remote.localApiKey)))) { minigraphLogger.debug('Creating local API key for Connect'); - const apiKeyService = new ApiKeyService(); - const localApiKey = await apiKeyService.createLocalConnectApiKey(); + const localApiKey = await service.createLocalConnectApiKey();Also applies to: 19-19
23-29: 🛠️ Refactor suggestionAdd error handling for store dispatch.
The store dispatch operation should include error handling to ensure configuration updates are successful.
- store.dispatch( - updateUserConfig({ - remote: { - localApiKey: localApiKey.key, - }, - }) - ); + try { + await store.dispatch( + updateUserConfig({ + remote: { + localApiKey: localApiKey.key, + }, + }) + ); + } catch (error) { + minigraphLogger.error('Failed to update configuration with new local API key', error); + throw new Error('Failed to save local API key to configuration'); + }
🧹 Nitpick comments (3)
api/src/unraid-api/auth/api-key.service.ts (3)
211-216: Improve security of API key comparison.The timing-safe comparison is correctly implemented for the 'key' field, but consider using a constant-time comparison function for all sensitive field comparisons to prevent timing attacks.
if (field === 'key') { if (crypto.timingSafeEqual(Buffer.from(apiKey[field]), Buffer.from(value))) { apiKey.roles = apiKey.roles.map((role) => role || Role.GUEST); return apiKey; } - } else if (apiKey[field] === value) { + } else if (field === 'id' || field === 'name') { + if (crypto.timingSafeEqual(Buffer.from(apiKey[field]), Buffer.from(value))) { apiKey.roles = apiKey.roles.map((role) => role || Role.GUEST); return apiKey; + } }
281-288: Consider adding atomic write operation.The current implementation of saveApiKey could lead to corrupted files if the process crashes during write. Consider using atomic write operations.
const sortedApiKey = Object.keys(validatedApiKey) .sort() .reduce((acc, key) => { acc[key] = validatedApiKey[key]; return acc; }, {} as ApiKeyWithSecret); - await writeFile(this.keyFile(validatedApiKey.id), JSON.stringify(sortedApiKey, null, 2)); + const tempFile = `${this.keyFile(validatedApiKey.id)}.tmp`; + await writeFile(tempFile, JSON.stringify(sortedApiKey, null, 2)); + await rename(tempFile, this.keyFile(validatedApiKey.id));
28-28: Consider implementing memory cache eviction.The
memoryApiKeysmap could potentially grow unbounded. Consider implementing a cache eviction strategy or maximum size limit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json(1 hunks)api/src/mothership/utils/create-local-connect-api-key.ts(1 hunks)api/src/unraid-api/auth/api-key.service.ts(8 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
api/src/unraid-api/auth/api-key.service.ts (1)
273-275: Review role assignment in createLocalConnectApiKey.
The method creates a Connect user with the CONNECT role, which aligns with the principle of least privilege. This is a good improvement from the previous ADMIN role assignment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
api/src/__test__/store/modules/config.test.ts (1)
Line range hint
1-162: Refactor: Extract test data to improve maintainabilityThe test data structure is duplicated across test cases. Consider extracting the expected state object into a shared test fixture to improve maintainability and reduce duplication.
Example implementation:
// test-fixtures/config.ts export const defaultConfigState = { api: { extraOrigins: 'https://google.com,https://test.com', version: expect.any(String), }, // ... rest of the test state }; // In your test file: import { defaultConfigState } from './test-fixtures/config'; test('After init returns values from cfg file for all fields', async () => { const { loadConfigFile } = await import('@app/store/modules/config'); await store.dispatch(loadConfigFile()); const state = store.getState().config; expect(state).toMatchObject(expect.objectContaining(defaultConfigState)); });api/src/__test__/graphql/resolvers/subscription/network.test.ts (2)
Line range hint
69-79: Consider simplifying the test configuration for better readability.While the test logic is correct, the configuration object could be simplified for better maintainability.
Consider this refactor:
-test('getUrlForServer - field does not exist, ssl disabled', async () => { - const getResult = async () => getUrlForServer( - { - nginx: { lanIp: '192.168.1.1', sslEnabled: false, sslMode: 'no' } as const as Nginx, - ports: { - port: ':123', portSsl: ':445', defaultUrl: new URL('https://my-default-url.unraid.net'), - }, - // @ts-expect-error Field doesn't exist - field: 'idontexist', - }); +test('getUrlForServer - field does not exist, ssl disabled', async () => { + const config = { + nginx: { lanIp: '192.168.1.1', sslEnabled: false, sslMode: 'no' } as const as Nginx, + ports: { + port: ':123', + portSsl: ':445', + defaultUrl: new URL('https://my-default-url.unraid.net'), + }, + // @ts-expect-error Field doesn't exist + field: 'idontexist', + }; + const getResult = async () => getUrlForServer(config);
107-113: Consider consolidating similar test cases using test.each.This test case is similar to the previous one (lines 69-79), differing only in configuration. Consider using
test.eachto reduce duplication and make the test suite more maintainable.Here's a suggested refactor:
+const invalidFieldTestCases = [ + { + name: 'ssl disabled with ports', + config: { + nginx: { lanIp: '192.168.1.1', sslEnabled: false, sslMode: 'no' } as const as Nginx, + ports: { port: ':123', portSsl: ':445', defaultUrl: new URL('https://my-default-url.unraid.net') }, + } + }, + { + name: 'with FQDN', + config: { + nginx: { lanFqdn: 'my-fqdn.unraid.net' } as const as Nginx, + ports: { portSsl: '', port: '', defaultUrl: new URL('https://my-default-url.unraid.net') }, + } + } +]; + +test.each(invalidFieldTestCases)('getUrlForServer - field does not exist - $name', async ({ config }) => { + const getResult = async () => getUrlForServer({ + ...config, + // @ts-expect-error Field doesn't exist + field: 'idontexist' + }); + await expect(getResult).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: IP URL Resolver: Could not resolve any access URL for field: "idontexist", is FQDN?: false]` + ); +});api/src/unraid-api/auth/api-key.service.ts (4)
28-28: Consider implementing caching with memoryApiKeys Map.The
memoryApiKeysproperty is declared but not utilized. Consider implementing a caching mechanism to improve performance by reducing file I/O operations.Additionally, implement the file watch functionality mentioned in the TODO comment to ensure cache consistency when files are modified externally.
Also applies to: 46-47
55-59: Consider expanding name validation for international characters.The current regex
^[\p{L}\p{N} ]+$allows letters, numbers, and spaces, but consider allowing additional characters like hyphens or underscores for more flexibility while maintaining security.-if (/^[\p{L}\p{N} ]+$/u.test(name)) { +if (/^[\p{L}\p{N} _-]+$/u.test(name)) {
211-215: Consider extracting timing-safe comparison to a separate method.The timing-safe comparison logic could be reused elsewhere and would benefit from being in a dedicated method.
+ private compareKeys(stored: string, provided: string): boolean { + return crypto.timingSafeEqual(Buffer.from(stored), Buffer.from(provided)); + } + if (field === 'key') { - if (crypto.timingSafeEqual(Buffer.from(apiKey[field]), Buffer.from(value))) { + if (this.compareKeys(apiKey[field], value)) { apiKey.roles = apiKey.roles.map((role) => role || Role.GUEST); return apiKey; }
281-288: Consider adding error context and validation.The sorting of properties is good for consistency, but consider:
- Adding validation for required fields before saving
- Including more context in error messages
const sortedApiKey = Object.keys(validatedApiKey) .sort() .reduce((acc, key) => { acc[key] = validatedApiKey[key]; return acc; }, {} as ApiKeyWithSecret); + // Validate required fields + const requiredFields = ['id', 'key', 'name', 'roles'] as const; + for (const field of requiredFields) { + if (!sortedApiKey[field]) { + throw new GraphQLError(`Failed to save API key: Missing required field '${field}'`); + } + } await writeFile(this.keyFile(validatedApiKey.id), JSON.stringify(sortedApiKey, null, 2));api/src/unraid-api/auth/api-key.service.spec.ts (1)
309-313: Add test cases for edge cases in key validation.Consider adding test cases for:
- Keys with special characters
- Empty or whitespace-only keys
- Very long keys
it('should handle edge cases in key validation', async () => { const cases = [ { key: ' '.repeat(100), expected: null }, { key: '!@#$%^&*()', expected: null }, { key: '', expected: null } ]; for (const testCase of cases) { const result = await apiKeyService.findByKey(testCase.key); expect(result).toBe(testCase.expected); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/src/__test__/graphql/resolvers/subscription/network.test.ts(3 hunks)api/src/__test__/store/modules/config.test.ts(4 hunks)api/src/unraid-api/auth/api-key.service.spec.ts(5 hunks)api/src/unraid-api/auth/api-key.service.ts(8 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
api/src/__test__/store/modules/config.test.ts
91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
148-148: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1)
61-66: LGTM! Proper async/await usage for promise rejection testing.
The test correctly handles the asynchronous nature of the URL generation failure case for SSL auto mode.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json (1)
1-10:⚠️ Potential issueCritical: Remove sensitive API key from source control
This file contains a sensitive API key that should not be committed to the repository.
🧰 Tools
🪛 Gitleaks (8.21.2)
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🧹 Nitpick comments (1)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json (1)
1-10: Enhance configuration structure with additional metadataWhile the current structure includes basic metadata, consider enhancing it with:
expiresAtfield to implement key rotationlastUsedtimestamp for auditingallowedIPsfor IP-based access controlrevokedAtfor key revocation trackingHere's a suggested enhanced structure:
{ "createdAt": "2024-12-19T17:32:59.531Z", "description": "API key for Connect user", "id": "d166bf8b-3615-444a-8932-c460b2132ba3", "key": "3a4e2332891e879d2ac8c3f25ef03a7b54f70b62cd6c5a08a86189cdd19ba203", "name": "Connect", "roles": [ "connect" - ] + ], + "expiresAt": "2025-12-19T17:32:59.531Z", + "lastUsed": null, + "allowedIPs": ["*"], + "revokedAt": null }🧰 Tools
🪛 Gitleaks (8.21.2)
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
api/src/unraid-api/auth/api-key.service.ts (3)
47-51: Improve error message clarity in sanitizeNameThe error message could be more descriptive about the allowed characters. Consider updating it to explicitly mention Unicode letters are supported.
- throw new GraphQLError('API key name must be alphanumeric + spaces'); + throw new GraphQLError('API key name must contain only letters, numbers, and spaces (Unicode letters are supported)');
Line range hint
180-236: Optimize findByField performance and error handlingThe current implementation has some areas for improvement:
- Reading all files sequentially can be slow with many keys
- Error handling could be more specific
Consider these improvements:
- Add an index file to optimize lookups
- Improve error handling granularity
async findByField(field: keyof ApiKeyWithSecret, value: string): Promise<ApiKeyWithSecret | null> { if (!value) return null; + + // Check index first if looking up by name + if (field === 'name' && this.keyIndex?.has(value)) { + return this.findById(this.keyIndex.get(value)!); + } try { const files = await readdir(this.basePath);
275-282: Optimize key sorting implementationThe current implementation creates an unnecessary intermediate object. Consider using
JSON.stringifywith a replacer function.- const sortedApiKey = Object.keys(validatedApiKey) - .sort() - .reduce((acc, key) => { - acc[key] = validatedApiKey[key]; - return acc; - }, {} as ApiKeyWithSecret); - - await writeFile(this.keyFile(validatedApiKey.id), JSON.stringify(sortedApiKey, null, 2)); + await writeFile( + this.keyFile(validatedApiKey.id), + JSON.stringify( + validatedApiKey, + Object.keys(validatedApiKey).sort(), + 2 + ) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/auth/api-key.service.ts(7 hunks)
🔇 Additional comments (1)
api/src/unraid-api/auth/api-key.service.ts (1)
267-269: LGTM! Correct role assignment for Connect user
The implementation correctly uses the CONNECT role instead of ADMIN, addressing the security concern raised in the previous review.
| private readonly logger = new Logger(ApiKeyService.name); | ||
| protected readonly basePath: string; | ||
| protected readonly keyFile: (id: string) => string; | ||
| protected memoryApiKeys = new Map<string, ApiKeyWithSecret>(); |
There was a problem hiding this comment.
Remove unused property memoryApiKeys
The memoryApiKeys Map is declared but never used throughout the service. Consider removing this property if it's not needed, or implement the in-memory caching functionality if that was the intended purpose.
| const existingKey = await this.findByField('name', sanitizedName); | ||
| if (!overwrite && existingKey) { | ||
| throw new GraphQLError('API key name already exists, use overwrite flag to update'); | ||
| } | ||
| const apiKey: Partial<ApiKeyWithSecret> = { | ||
| id: uuidv4(), | ||
| key: this.generateApiKey(), | ||
| name: sanitizedName, | ||
| description, | ||
| roles, | ||
| createdAt: new Date().toISOString(), | ||
| ...(existingKey ?? {}), | ||
| }; | ||
|
|
||
| await this.saveApiKey(apiKey); | ||
| apiKey.description = description; | ||
| apiKey.roles = roles; | ||
| // Update createdAt date | ||
| apiKey.createdAt = new Date().toISOString(); | ||
|
|
||
| await this.saveApiKey(apiKey as ApiKeyWithSecret); | ||
|
|
||
| return apiKey; | ||
| return apiKey as ApiKeyWithSecret; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for generated API key format
The generated API key should be validated to ensure it meets the expected format before saving.
const apiKey: Partial<ApiKeyWithSecret> = {
id: uuidv4(),
- key: this.generateApiKey(),
+ key: await this.generateAndValidateApiKey(),
name: sanitizedName,
...(existingKey ?? {}),
};
+ private async generateAndValidateApiKey(): Promise<string> {
+ const key = this.generateApiKey();
+ if (!/^[a-f0-9]{64}$/.test(key)) {
+ throw new GraphQLError('Failed to generate valid API key');
+ }
+ return key;
+ }Committable suggestion skipped: line range outside the PR's diff.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
force-pushed
the
feat/generate-key-once
branch
from
fba8133 to
bdc3e47
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/auth/api-key.service.ts (3)
85-103: Consider adding more specific error messages for validation failuresWhile the implementation is solid, the error messages could be more specific about which validation failed.
Consider this improvement:
- throw new GraphQLError('API key name already exists, use overwrite flag to update'); + throw new GraphQLError(`API key '${sanitizedName}' already exists. Use overwrite=true to update it.`);
Line range hint
190-240: Consider implementing caching for frequently accessed keysThe implementation is secure with timing-safe comparison, but reading from disk for every lookup could impact performance.
Consider utilizing the existing
memoryApiKeysMap for caching:async findByField(field: keyof ApiKeyWithSecret, value: string): Promise<ApiKeyWithSecret | null> { if (!value) return null; + + // Check cache first for key lookups + if (field === 'key' && this.memoryApiKeys.has(value)) { + return this.memoryApiKeys.get(value) || null; + } try { const files = await readdir(this.basePath);
285-292: Consider implementing atomic write operationsWhile the property sorting ensures consistent file format, the file write operation could fail partially, leaving the file in an inconsistent state.
Consider using atomic write operations:
- await writeFile(this.keyFile(validatedApiKey.id), JSON.stringify(sortedApiKey, null, 2)); + const tempFile = `${this.keyFile(validatedApiKey.id)}.tmp`; + await writeFile(tempFile, JSON.stringify(sortedApiKey, null, 2)); + await rename(tempFile, this.keyFile(validatedApiKey.id));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/auth/api-key.service.spec.ts(5 hunks)api/src/unraid-api/auth/api-key.service.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/auth/api-key.service.spec.ts
🔇 Additional comments (4)
api/src/unraid-api/auth/api-key.service.ts (4)
28-28: Remove unused property memoryApiKeys
The memoryApiKeys Map is declared but never used throughout the service. Consider removing this property if it's not needed, or implement the in-memory caching functionality if that was the intended purpose.
55-61: LGTM! Robust name validation implementation.
The Unicode-aware validation ensures proper handling of international characters while maintaining security.
244-246: LGTM! Clean implementation.
Good use of code reuse by delegating to findByField.
277-279: LGTM! Proper role assignment for Connect API key.
The implementation correctly uses the CONNECT role instead of ADMIN, addressing previous security concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/src/mothership/utils/create-local-connect-api-key.ts (3)
8-10: Consider returning a status or success indicator.
Currently, the function logs and returns early if the config file is not loaded. For better testability and clarity, you might consider returning a boolean (e.g.,false) to denote the API key was not created.
16-16: Validate presence ofremote.apikey.
Ifremote.apikeyis expected to be a string (and not an empty string), you could add an additional check to ensure it’s not an empty value. This makes failure scenarios unambiguous.
30-30: Consider using more descriptive error classes.
Instead ofthrow new Error(...), consider throwing a domain-specific error orGraphQLErrorfor improved consistency and error distinction.api/src/unraid-api/auth/casbin/policy.ts (1)
18-18: Role inheritance caution.
By inheriting fromGUEST, theCONNECTrole may gain additional resource restrictions or grants intended forGUEST. Consider whether a standalone role or selective inheritance is more appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (17)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json(1 hunks)api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json(1 hunks)api/dev/states/myservers.cfg(2 hunks)api/src/__test__/graphql/resolvers/subscription/network.test.ts(3 hunks)api/src/__test__/store/modules/config.test.ts(4 hunks)api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts(2 hunks)api/src/graphql/schema/types/auth/auth.graphql(0 hunks)api/src/graphql/schema/types/auth/roles.graphql(1 hunks)api/src/index.ts(2 hunks)api/src/mothership/utils/create-local-connect-api-key.ts(1 hunks)api/src/store/listeners/listener-middleware.ts(2 hunks)api/src/store/listeners/local-api-key-listener.ts(0 hunks)api/src/unraid-api/auth/api-key.service.spec.ts(5 hunks)api/src/unraid-api/auth/api-key.service.ts(8 hunks)api/src/unraid-api/auth/casbin/policy.ts(1 hunks)api/src/unraid-api/graph/connect/connect.resolver.ts(3 hunks)
💤 Files with no reviewable changes (2)
- api/src/graphql/schema/types/auth/auth.graphql
- api/src/store/listeners/local-api-key-listener.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- api/dev/Unraid.net/myservers.cfg
- api/src/graphql/schema/types/auth/roles.graphql
- api/dev/keys/10f356da-1e9e-43b8-9028-a26a645539a6.json
- api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts
- api/dev/keys/d166bf8b-3615-444a-8932-c460b2132ba3.json
- api/src/index.ts
- api/src/test/store/modules/config.test.ts
- api/dev/states/myservers.cfg
- api/src/store/listeners/listener-middleware.ts
- api/src/unraid-api/graph/connect/connect.resolver.ts
- api/src/test/graphql/resolvers/subscription/network.test.ts
- api/src/unraid-api/auth/api-key.service.spec.ts
🧰 Additional context used
📓 Learnings (1)
api/src/mothership/utils/create-local-connect-api-key.ts (1)
Learnt from: elibosley
PR: unraid/api#992
File: api/src/mothership/utils/create-local-connect-api-key.ts:23-29
Timestamp: 2024-12-20T14:56:29.464Z
Learning: The user prefers not to add error handling for the Redux dispatch in create-local-connect-api-key.ts.
🔇 Additional comments (8)
api/src/unraid-api/auth/casbin/policy.ts (1)
9-11: Double-check scope of read permissions.
Granting READ_ANY on all resources to the CONNECT role might inadvertently expose sensitive data if not carefully filtered at the application layer. Ensure that everything is authorized as intended.
api/src/unraid-api/auth/api-key.service.ts (7)
28-28: Remove unused property or implement in-memory caching.
It appears memoryApiKeys is declared but nowhere utilized. This was previously noted, so marking as duplicate.
55-61: Unicode name validation looks good.
This additional validation prevents invalid API key names and is aligned with the requirement to accept letters, numbers, and spaces.
67-68: Role checks are correctly handled.
The newly introduced logic for verifying valid roles prevents malformed or unauthorized role assignments.
85-89: Efficient approach for handling existing keys.
Using findByField('name', ...) before creation helps prevent duplicates when overwrite is false. This is a clear, concise approach.
215-219: Good use of timing-safe comparison.
crypto.timingSafeEqual helps prevent timing side-channel attacks when comparing sensitive strings.
277-279: Concise helper for Connect-specific keys.
Using a dedicated createLocalConnectApiKey method highlights the unique role and use case for Connect.
285-291: Sorting attributes upon save is a neat approach.
Sorting the API key properties before saving can help maintain consistent and predictable JSON structures.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| const sortedApiKey = Object.keys(validatedApiKey) | ||
| .sort() | ||
| .reduce((acc, key) => { | ||
| acc[key] = validatedApiKey[key]; | ||
| return acc; | ||
| }, {} as ApiKeyWithSecret); |
There was a problem hiding this comment.
should we consider pulling this into a utility at some point?
There was a problem hiding this comment.
I think that since this is part of the saveApiKey service anyway, it's kind of already in a service. That being said I'd be open to a "sortObject" function as a utility eventually if that's what you're suggesting.
| # UPC permissions for API keys | ||
| p, ${Role.UPC}, ${Resource.API_KEY}, ${AuthAction.CREATE_ANY} | ||
| p, ${Role.UPC}, ${Resource.API_KEY}, ${AuthAction.UPDATE_ANY} | ||
| # UPC permissions | ||
| p, ${Role.UPC}, ${Resource.CLOUD}, ${AuthAction.READ_ANY} |
There was a problem hiding this comment.
just making sure that removing upc & myserver policies is intended!
There was a problem hiding this comment.
I'm thinking this may break session auth if we don't update the getSessionUser() method in the auth.service unless that has been changed already in another PR.
There was a problem hiding this comment.
Great callout. I'll change getSessionUser() to return an administrator.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/auth/api-key.service.ts (1)
277-282: Consider extracting the sorting logic to a utility function.The property sorting logic could be moved to a shared utility function for reuse across the codebase, as it might be useful for other JSON operations.
// utils/object.ts export function sortObjectProperties<T extends object>(obj: T): T { return Object.keys(obj) .sort() .reduce((acc, key) => { acc[key] = obj[key]; return acc; }, {} as T); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
api/src/__test__/store/modules/config.test.ts(2 hunks)api/src/graphql/schema/types/auth/roles.graphql(1 hunks)api/src/unraid-api/auth/api-key.service.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/graphql/schema/types/auth/roles.graphql
- api/src/test/store/modules/config.test.ts
🔇 Additional comments (7)
api/src/unraid-api/auth/api-key.service.ts (7)
20-20: Remove unused propertymemoryApiKeysThe
memoryApiKeysMap is declared but never used throughout the service. Consider removing this property if it's not needed, or implement the in-memory caching functionality if that was the intended purpose.
47-53: LGTM! Robust Unicode-aware name validation.The validation logic effectively handles Unicode characters while maintaining security by restricting to letters, numbers, and spaces.
59-60: LGTM! Well-implemented overwrite functionality.The overwrite parameter provides good control over API key updates with proper validation and immutable state handling. The implementation follows best practices by:
- Checking existence before creation
- Using immutable operations
- Maintaining audit trail with updated timestamps
Also applies to: 77-95
Line range hint
182-232: LGTM! Secure implementation with timing attack protection.Excellent implementation with security considerations:
- Uses timing-safe comparison for API keys to prevent timing attacks
- Handles corrupted files gracefully
- Provides proper error messages for system issues
236-238: LGTM! Clean method delegation.Good refactoring that follows the DRY principle by reusing the
findByFieldimplementation.
269-271: LGTM! Proper role assignment for Connect API key.The implementation correctly uses the CONNECT role and ensures single key existence through the overwrite flag.
284-284: LGTM! Comprehensive error handling.The error handling is well-implemented with specific messages for different error types, making debugging easier.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/auth/auth.service.spec.ts (1)
143-143: Consider adding test coverage for CONNECT role scenarios.While the test logic remains valid, consider adding specific test cases for the new CONNECT role to ensure proper handling of the new role type.
Example test cases to consider:
- Adding CONNECT role to an API key with existing roles
- Removing CONNECT role from an API key
- Validating permissions specific to the CONNECT role
Also applies to: 155-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/auth/auth.service.spec.ts(4 hunks)api/src/unraid-api/auth/auth.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/src/unraid-api/auth/auth.service.ts
🔇 Additional comments (3)
api/src/unraid-api/auth/auth.service.spec.ts (3)
42-42: LGTM! Role update maintains consistency.The mockUser roles have been correctly updated to match the new role structure.
137-137: Verify if ADMIN role is the intended test case.The role has been changed from UPC to ADMIN in this test case. Please confirm if this change aligns with the test's purpose of validating role addition functionality.
Could you clarify if this change is intentional and if it affects the test coverage of the new CONNECT role scenarios?
25-25: LGTM! Verify role consistency across tests.The role update from UPC to CONNECT aligns with the broader changes in the codebase.
Let's verify the role changes are consistent across the codebase:
✅ Verification successful
Role migration from UPC to CONNECT is complete and consistent
The changes in
auth.service.spec.tsare part of a complete and consistent migration:
- No remaining instances of Role.UPC in the codebase
- Role.CONNECT is properly integrated with defined permissions and role hierarchy
- Test cases correctly reflect the new role structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of Role.UPC to ensure complete migration rg "Role\.UPC" --type ts # Search for the new Role.CONNECT usage pattern rg "Role\.CONNECT" --type tsLength of output: 655
elibosley
force-pushed
the
feat/generate-key-once
branch
from
fe87012 to
a471c35
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Role.CONNECT, enabling broader access to resources.ResourceandRoleadded to the GraphQL schema.Bug Fixes
Refactor
getSessionUsermethod to return an account with administrator permissions.Chores