feat: add cli command for api keys #1063
Conversation
WalkthroughThis pull request introduces significant enhancements to the Unraid API's authentication and configuration management system, emphasizing Single Sign-On (SSO) functionality. The changes encompass various components across the API, CLI, web interface, and plugin systems, implementing new methods for managing API keys, SSO users, and refining the overall authentication flow. Key modifications include restructuring configuration handling, adding SSO-related commands, and updating type definitions to accommodate these new features. Changes
Sequence DiagramsequenceDiagram
participant User
participant SSOButton
participant AuthServer
participant UnraidAPI
participant ConfigStore
User->>SSOButton: Click SSO Login
SSOButton->>AuthServer: Generate Authorization Request
AuthServer-->>SSOButton: Return Authorization Code
SSOButton->>UnraidAPI: Exchange Code for Token
UnraidAPI->>ConfigStore: Store SSO User ID
UnraidAPI-->>SSOButton: Authentication Success
SSOButton->>User: Enable Login
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (5)
api/src/cli/commands/key.ts (3)
12-19: Consider using Role enum for roles type.The
rolesfield inKeyFlagscould be more strictly typed to ensure type safety at compile time.type KeyFlags = { create?: boolean; command: string; description?: string; name: string; permissions?: string; - roles?: string; + roles?: `${Role}` | `${Role},${Role}`[]; };
23-40: Enhance role validation with empty string handling.The
validateRolesfunction should handle empty strings explicitly to prevent potential issues.const validateRoles = (rolesStr?: string): Role[] => { - if (!rolesStr) return [Role.GUEST]; + if (!rolesStr?.trim()) return [Role.GUEST]; const requestedRoles = rolesStr.split(',').map((role) => role.trim().toUpperCase() as Role);
53-58: Enhance permissions documentation.The permissions description could be more detailed to help users understand the expected format and available options.
permissions: { type: String, optional: true, - description: 'Comma-separated list of permissions', + description: 'Comma-separated list of permissions (e.g., read:any, write:own)', typeLabel: '{underline perm1,perm2}', },api/README.md (2)
46-48: Add language specifiers to code blocks.Add
bashlanguage specifier to code blocks for proper syntax highlighting.-``` +```bash unraid-api key get --name "my-app-key"Also applies to: 52-54, 58-60, 64-66 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 46-46: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `89-90`: **Fix grammatical issues in the Secrets section.** Add missing commas for better readability. ```diff -If you found this file you're likely a developer. If you'd like to know more about the API and when it's available please join [our discord](https://discord.unraid.net/). +If you found this file, you're likely a developer. If you'd like to know more about the API and when it's available, please join [our discord](https://discord.unraid.net/).🧰 Tools
🪛 LanguageTool
[uncategorized] ~90-~90: Possible missing comma found.
Context: ... -vv ``` ## Secrets If you found this file you're likely a developer. If you'd lik...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~90-~90: It seems that a comma is missing.
Context: ...re about the API and when it's available please join [our discord](https://discord.unra...(IF_PLEASE_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/README.md(1 hunks)api/src/cli/commands/key.ts(1 hunks)api/src/cli/index.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
api/README.md
[uncategorized] ~90-~90: Possible missing comma found.
Context: ... -vv ``` ## Secrets If you found this file you're likely a developer. If you'd lik...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~90-~90: It seems that a comma is missing.
Context: ...re about the API and when it's available please join [our discord](https://discord.unra...
(IF_PLEASE_COMMA)
🪛 Markdownlint (0.37.0)
api/README.md
46-46: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
52-52: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
64-64: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
72-72: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
78-78: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
84-84: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/cli/index.ts (1)
33-33: LGTM! Command registration follows existing patterns.The new
keycommand is properly integrated into the CLI command structure, maintaining consistency with other command imports.api/src/cli/commands/key.ts (1)
73-78:⚠️ Potential issuePermissions parameter is not utilized in key creation.
The API key creation accepts a permissions parameter in the CLI options but doesn't pass it to the
apiKeyService.createmethod.Let's verify the ApiKeyService implementation:
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/package.json (1)
26-26: Improve the robustness of the command script.The script addition looks good but could be more robust with these improvements:
- Use cross-platform clear command
- Add error handling for build step
- Use node path resolution instead of relative path
Consider this improved version:
-"command": "npm run build && clear && ./dist/cli.js", +"command": "npm run build && (clear || cls) && node dist/cli.js",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/package.json (1)
26-26: Verify CLI executable permissions.Since this script executes
dist/cli.jsdirectly, verify that the postbuild script correctly sets the executable permissions.✅ Verification successful
Executable permissions are properly configured ✅
The postbuild script automatically sets executable permissions for
dist/cli.jsafter each build:"postbuild": "chmod +x dist/main.js && chmod +x dist/cli.js"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if postbuild script sets executable permissions for cli.js # Test: Check if postbuild script exists and sets permissions rg -A 1 "postbuild.*chmod.*cli\.js" package.json # Test: Verify executable permissions are set after build ls -l dist/cli.js 2>/dev/null || echo "dist/cli.js not found. Run build first."Length of output: 212
Script:
#!/bin/bash # Check api/package.json for build and permission-related scripts cat api/package.json | jq '.scripts' # Also search for any chmod commands in case they're in a different script rg -A 1 "chmod.*cli\.js" api/Length of output: 1550
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (14)
api/src/cli.ts (2)
19-19: Trim shell path to avoid whitespace issuesThe output of
execSync('which bash')may include trailing newline characters or whitespace, which can cause issues when used as an executable path.Update the code to trim the shell path:
nativeShell: { executablePath: shellToUse.toString('utf-8') }, +nativeShell: { executablePath: shellToUse.toString('utf-8').trim() },
23-23: Log error messages instead of error objectsLogging the entire error object with
cliLogger.error('ERROR:', error);may expose sensitive information. It's safer to log only the error message.Adjust the error logging:
-cliLogger.error('ERROR:', error); +cliLogger.error('ERROR:', error.message);api/src/unraid-api/cli/switch-env.command.ts (1)
6-6: Inconsistent logger usage: Usethis.loggerconsistentlyThe code uses both
this.loggerandcliLoggerwithin the same class. For consistency and to adhere to best practices, consider usingthis.loggerthroughout the class.Apply this diff to replace
cliLoggerwiththis.loggerand remove the unused import:- import { cliLogger } from '@app/core/log'; + // Removed unused cliLogger import ... ...Also applies to: 83-83, 86-86
api/src/unraid-api/cli/key.command.ts (1)
5-5: Inconsistent logger usage: Usethis.loggerconsistentlyThe code imports and uses both
cliLoggerandthis.logger. To maintain consistency and ensure proper logging practices, it's advisable to usethis.loggerthroughout the class.Apply this diff to replace
cliLoggerwiththis.loggerand remove the unnecessary import:- import { cliLogger } from '@app/core/log'; + // Removed unused cliLogger import ...Also applies to: 46-46
api/src/unraid-api/cli/version.command.ts (2)
6-6: Add command description for consistency.Add a description to the Command decorator to maintain consistency with other commands.
-@Command({ name: 'version' }) +@Command({ name: 'version', description: 'Display Unraid API version' })
11-13: Add version validation.Consider validating API_VERSION before logging to handle undefined or invalid versions gracefully.
async run(): Promise<void> { + if (!API_VERSION) { + this.logger.error('API version is not configured'); + process.exit(1); + } this.logger.info(`Unraid API v${API_VERSION}`); }api/src/unraid-api/cli/report.spec.ts (3)
12-14: Add more test cases for comprehensive coverage.The current test only covers a basic socket origin case. Consider adding tests for:
- Multiple socket origins
- Invalid socket paths
- Empty array input
test('anonymise origins handles multiple socket origins', () => { expect(anonymiseOrigins([ '/var/run/test1.sock', '/var/run/test2.sock' ])).toEqual([]); }); test('anonymise origins handles invalid socket paths', () => { expect(anonymiseOrigins(['invalid.sock'])).toEqual(['invalid.sock']); }); test('anonymise origins handles empty input', () => { expect(anonymiseOrigins([])).toEqual([]); });
16-18: Expand WAN port anonymization test cases.Add more test cases to cover:
- Multiple ports
- Invalid URLs
- URLs without ports
test('anonymise origins handles various URL formats', () => { expect(anonymiseOrigins([ 'https://domain.tld:8443', 'http://another.domain:9000', 'https://no.port.domain', 'invalid:url:format' ])).toEqual([ 'https://domain.tld:WANPORT', 'http://another.domain:WANPORT', 'https://no.port.domain', 'invalid:url:format' ]); });
7-10: Consider using beforeEach instead of beforeAll.Using
beforeEachwould ensure a clean store state for each test case and prevent potential test interdependencies.-beforeAll(async () => { +beforeEach(async () => { // Load cfg into store await store.dispatch(loadConfigFile()); });api/src/unraid-api/cli/log.service.ts (2)
5-5: Consider using a proper logging libraryUsing console directly limits logging capabilities. Consider using Winston or Pino for:
- Structured logging
- Log levels configuration
- Output formatting
- File transport
27-29: Improve type safety for debug methodThe debug method accepts
anytype which bypasses TypeScript's type checking. Consider adding proper typing.- debug(message: any, ...optionalParams: any[]): void { + debug(message: string | Error, ...optionalParams: unknown[]): void { this.logger.debug(message, ...optionalParams); }api/src/unraid-api/cli/cli.module.ts (1)
15-26: Consider grouping related commandsThe commands could be organized into groups for better maintainability. Consider:
- Using command groups in nest-commander
- Adding documentation for each command's purpose
- Organizing commands by functionality (e.g., process management, reporting, authentication)
Example grouping structure:
@Module({ providers: [ LogService, // Process Management StartCommand, StopCommand, RestartCommand, // Reporting ReportCommand, StatusCommand, // Authentication KeyCommand, ValidateTokenCommand, // Configuration SwitchEnvCommand, VersionCommand, ], })api/src/unraid-api/cli/start.command.ts (1)
44-48: Improve type safety in parseLogLevelThe type casting can be improved using type predicates.
- parseLogLevel(val: unknown): typeof levels { - return (levels.includes(val as (typeof levels)[number]) - ? (val as (typeof levels)[number]) - : 'info') as unknown as typeof levels; - } + parseLogLevel(val: unknown): (typeof levels)[number] { + const isValidLevel = (v: unknown): v is (typeof levels)[number] => + typeof v === 'string' && levels.includes(v as (typeof levels)[number]); + return isValidLevel(val) ? val : 'info'; + }api/src/unraid-api/cli/validate-token.command.ts (1)
36-48: Consider adding timeout for remote validation.The remote JWKS validation could potentially hang if the remote server is unresponsive.
Consider adding a timeout option to the remote JWKS validation:
try { this.logger.debug('Local validation failed for key, trying remote validation'); - tokenPayload = (await jwtVerify(token, this.JWKSOnline)).payload; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + tokenPayload = (await jwtVerify(token, this.JWKSOnline, { + signal: controller.signal + })).payload; + clearTimeout(timeoutId); } catch (error: unknown) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
api/package.json(2 hunks)api/src/__test__/cli/commands/report/anonymise-origins.test.ts(0 hunks)api/src/__test__/cli/log-to-syslog.test.ts(0 hunks)api/src/cli.ts(1 hunks)api/src/cli/commands/no-op.ts(0 hunks)api/src/cli/commands/report.ts(0 hunks)api/src/cli/commands/restart.ts(0 hunks)api/src/cli/commands/start.ts(0 hunks)api/src/cli/commands/status.ts(0 hunks)api/src/cli/commands/stop.ts(0 hunks)api/src/cli/commands/switch-env.ts(0 hunks)api/src/cli/commands/validate-token.ts(0 hunks)api/src/cli/commands/version.ts(0 hunks)api/src/cli/index.ts(0 hunks)api/src/cli/log-to-syslog.ts(0 hunks)api/src/cli/options.ts(0 hunks)api/src/cli/set-env.ts(0 hunks)api/src/core/log.ts(0 hunks)api/src/unraid-api/cli/cli.module.ts(1 hunks)api/src/unraid-api/cli/key.command.ts(1 hunks)api/src/unraid-api/cli/log.service.ts(1 hunks)api/src/unraid-api/cli/report.command.ts(1 hunks)api/src/unraid-api/cli/report.spec.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/status.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/cli/switch-env.command.ts(1 hunks)api/src/unraid-api/cli/validate-token.command.ts(1 hunks)api/src/unraid-api/cli/version.command.ts(1 hunks)api/src/unraid-api/rest/rest.service.ts(2 hunks)
💤 Files with no reviewable changes (16)
- api/src/cli/commands/version.ts
- api/src/cli/commands/stop.ts
- api/src/cli/commands/no-op.ts
- api/src/core/log.ts
- api/src/cli/set-env.ts
- api/src/cli/commands/status.ts
- api/src/cli/commands/start.ts
- api/src/cli/log-to-syslog.ts
- api/src/cli/index.ts
- api/src/test/cli/commands/report/anonymise-origins.test.ts
- api/src/cli/commands/restart.ts
- api/src/cli/options.ts
- api/src/test/cli/log-to-syslog.test.ts
- api/src/cli/commands/validate-token.ts
- api/src/cli/commands/switch-env.ts
- api/src/cli/commands/report.ts
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/cli/report.command.ts
[error] 237-237: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/src/unraid-api/cli/report.command.ts (1)
333-333: Verify the existence ofclearmethod inLogServiceThe
this.logger.clear()method is used, but it's not a standard method in typical logging services. Ensure thatLogServicehas aclearmethod or replace it with an appropriate function.Please run the following script to check if the
clearmethod exists inLogService:Also applies to: 358-358
api/src/unraid-api/rest/rest.service.ts (2)
18-28: LGTM! Improved error handling and report generation.The refactored implementation properly utilizes the new ReportCommand class and includes better error handling with detailed logging.
Line range hint
30-55: Verify tar command execution security.The tar command execution could potentially be vulnerable to command injection if the logPath contains malicious characters.
Run this script to check for potential command injection vulnerabilities:
✅ Verification successful
The tar command execution is secure against command injection
The
logPathis derived from a hardcoded system path/var/log/unraid-api/that cannot be influenced by user input. The path is properly normalized using Node.js's path resolution, making the tar command execution safe from command injection vulnerabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other tar command executions and their input validation rg -A 5 "execa\(['\"]\s*tar\s*['\"]" # Search for input validation on logPath ast-grep --pattern 'getters.paths()["log-base"]'Length of output: 609
Script:
#!/bin/bash # Find implementation of getters.paths() ast-grep --pattern 'paths() { $$$ }' # Search for path-related utility functions rg -A 5 "sanitizePath|validatePath|normalizePath" # Search for log-base configuration rg -A 3 '"log-base"'Length of output: 364
Script:
#!/bin/bash # Find paths module implementation fd paths.ts --exec cat {} \; # Look for path configuration rg -A 5 "paths.*=.*{" --type typescript # Check for path validation in store modules fd "store/modules" --type f --exec cat {} \;Length of output: 3660
api/package.json (1)
26-26: LGTM! Proper CLI tooling upgrade.The transition from ts-command-line-args to nest-commander is a good choice as it provides better integration with NestJS and more robust command handling capabilities.
Also applies to: 103-103
api/src/cli.ts
Outdated
| const shellToUse = execSync('which bash'); | ||
| await CommandFactory.run(CliModule, { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure error handling for the execSync command
The code uses execSync('which bash') to determine the shell path but does not handle the case where bash may not be available, which could lead to an unhandled exception.
Consider adding error handling to gracefully handle this scenario:
+let shellPath;
try {
const shellToUse = execSync('which bash');
- const shellPath = shellToUse.toString('utf-8').trim();
+ shellPath = shellToUse.toString('utf-8').trim();
+ if (!shellPath) {
+ throw new Error('Bash shell not found on the system.');
+ }
} catch (error) {
cliLogger.error('ERROR:', error.message);
internalLogger.error({
message: 'Failed to determine shell path',
error,
});
process.exit(1);
}
-await CommandFactory.run(CliModule, {
+await CommandFactory.run(CliModule, {
// ...
nativeShell: { executablePath: shellPath },
});Committable suggestion skipped: line range outside the PR's diff.
| await writeFile(envFlashFilePath, newEnvLine); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations to improve robustness
The writeFile and copyFile operations may fail due to issues like insufficient permissions or missing files. To enhance the robustness of the application, consider adding error handling around these file operations.
Apply this diff to add try-catch blocks with appropriate error logging:
+ try {
+ } catch (error) {
+ this.logger.error(`Failed to write to ${envFlashFilePath}:`, error);
+ throw error;
+ }
...
+ try {
+ } catch (error) {
+ this.logger.error(`Failed to copy from ${source} to ${destination}:`, error);
+ throw error;
+ }Also applies to: 84-85
|
|
||
| @Command({ name: 'key', arguments: '<name>' }) | ||
| export class KeyCommand extends CommandRunner { | ||
| private readonly logger = new Logger(KeyCommand.name); |
There was a problem hiding this comment.
Use dependency injection for ApiKeyService
Direct instantiation of services bypasses NestJS's dependency injection, leading to potential issues with dependencies and testing. Inject ApiKeyService through the constructor instead.
Apply this diff to use dependency injection:
...
...
...Also applies to: 71-71, 75-75, 89-89
| } | ||
|
|
||
| async run(passedParams: string[], options?: KeyOptions): Promise<void> { | ||
| console.log(options, passedParams); |
There was a problem hiding this comment.
Remove console.log statements from production code
Using console.log is discouraged in production code as it can lead to uncontrolled outputs. Replace it with the appropriate logging mechanism.
Apply this diff to remove the console.log statement:
Committable suggestion skipped: line range outside the PR's diff.
| execSync( | ||
| `${PM2_PATH} restart ${join(import.meta.dirname, '../../', 'ecosystem.config.json')} --update-env`, | ||
| { | ||
| env: process.env, | ||
| stdio: 'inherit', | ||
| cwd: process.cwd(), | ||
| } | ||
| ); |
There was a problem hiding this comment.
Add error handling for PM2 command execution
The current implementation using execSync could fail silently or expose sensitive information. Consider:
- Wrapping the execution in try-catch
- Sanitizing the output before logging
- Handling PM2 specific error codes
- execSync(
- `${PM2_PATH} restart ${join(import.meta.dirname, '../../', 'ecosystem.config.json')} --update-env`,
- {
- env: process.env,
- stdio: 'inherit',
- cwd: process.cwd(),
- }
- );
+ try {
+ execSync(
+ `${PM2_PATH} restart ${join(import.meta.dirname, '../../', 'ecosystem.config.json')} --update-env`,
+ {
+ env: process.env,
+ stdio: ['pipe', 'pipe', 'pipe'],
+ cwd: process.cwd(),
+ }
+ );
+ } catch (error) {
+ throw new Error(`Failed to restart API: ${error.message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| execSync( | |
| `${PM2_PATH} restart ${join(import.meta.dirname, '../../', 'ecosystem.config.json')} --update-env`, | |
| { | |
| env: process.env, | |
| stdio: 'inherit', | |
| cwd: process.cwd(), | |
| } | |
| ); | |
| try { | |
| execSync( | |
| `${PM2_PATH} restart ${join(import.meta.dirname, '../../', 'ecosystem.config.json')} --update-env`, | |
| { | |
| env: process.env, | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| cwd: process.cwd(), | |
| } | |
| ); | |
| } catch (error) { | |
| throw new Error(`Failed to restart API: ${error.message}`); | |
| } |
| execSync( | ||
| `${PM2_PATH} start ${join(import.meta.dirname, '../../', 'ecosystem.config.json')} --update-env`, | ||
| { | ||
| env: process.env, | ||
| stdio: 'inherit', | ||
| cwd: process.cwd(), | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract PM2 execution logic to a shared service
The PM2 execution logic is duplicated across commands. Consider creating a shared PM2 service.
// pm2.service.ts
@Injectable()
export class PM2Service {
constructor(private readonly logger: LogService) {}
async executeCommand(command: 'start' | 'stop' | 'restart'): Promise<void> {
const configPath = join(import.meta.dirname, '../../', 'ecosystem.config.json');
try {
execSync(`${PM2_PATH} ${command} ${configPath} --update-env`, {
env: process.env,
stdio: ['pipe', 'pipe', 'pipe'],
cwd: process.cwd(),
});
} catch (error) {
throw new Error(`Failed to ${command} API: ${error.message}`);
}
}
}
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
force-pushed
the
feat/add-cli-command-for-api-keys
branch
from
83e64ba to
0fc092c
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/src/unraid-api/cli/start.command.ts (1)
24-35: 🛠️ Refactor suggestionImprove error handling and environment variable handling.
Several issues to address:
- Environment variable concatenation is error-prone
- Error handling could be more graceful
- Previous review comment about PM2 service extraction is still valid
Consider this improved implementation:
async run(_, options: StartCommandOptions): Promise<void> { this.logger.info('Starting the Unraid API'); try { const env = { ...process.env, ...(options['log-level'] && { LOG_LEVEL: options['log-level'] }) }; const { stderr, stdout } = await execa(PM2_PATH, ['start', ECOSYSTEM_PATH, '--update-env'], { env } ); if (stdout) { this.logger.log(stdout); } if (stderr) { throw new Error(stderr); } } catch (error) { this.logger.error(`Failed to start API: ${error.message}`); process.exit(1); } }Previous suggestion to extract PM2 execution logic to a shared service still applies.
🧹 Nitpick comments (2)
api/src/unraid-api/cli/start.command.ts (2)
11-16: Consider adding validation for command options.The interface defines optional parameters but there's no validation for port number range or environment values.
Consider adding validation decorators:
interface StartCommandOptions { debug?: boolean; @IsPort() port?: string; 'log-level'?: string; @IsIn(['development', 'production', 'test']) environment?: string; }
41-45: Simplify type casting in parseLogLevel.The type casting can be simplified while maintaining type safety.
parseLogLevel(val: string): (typeof levels)[number] { return levels.includes(val as (typeof levels)[number]) ? val as (typeof levels)[number] : 'info'; }
🛑 Comments failed to post (2)
api/src/unraid-api/cli/logs.command.ts (2)
17-21: 🛠️ Refactor suggestion
Enhance input validation and parsing safety.
The current implementation has several areas for improvement:
- Uses unsafe
isNaNwhich attempts type coercion- Lacks validation for negative numbers
- Could benefit from more robust number parsing
Consider this improved implementation:
@Option({ flags: '-l, --lines', description: 'Number of lines to tail'}) parseLines(input: string): number { - return isNaN(parseInt(input)) ? 100 : parseInt(input) + const parsed = Number(input); + if (Number.isNaN(parsed) || !Number.isInteger(parsed) || parsed <= 0) { + this.logger.warn(`Invalid line count "${input}", using default of 100`); + return 100; + } + return parsed; }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
23-36: 🛠️ Refactor suggestion
Add robust error handling and process management.
The current implementation could benefit from improved error handling and process cleanup:
- No error handling for subprocess execution failures
- No cleanup of subprocess on errors
- No handling of process termination signals
Consider this enhanced implementation:
async run(passedParams: string[], options?: LogsOptions): Promise<void> { const lines = options?.lines ?? 100; - const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); + let subprocess: execa.ExecaChildProcess; + + try { + subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); + + subprocess.stdout?.on('data', (data) => { + this.logger.log(data.toString()); + }); + + subprocess.stderr?.on('data', (data) => { + this.logger.error(data.toString()); + }); + + // Handle process termination signals + const cleanup = () => { + if (subprocess) { + subprocess.kill(); + } + }; + + process.on('SIGINT', cleanup); + process.on('SIGTERM', cleanup); + + await subprocess; + } catch (error) { + this.logger.error(`Failed to execute logs command: ${error.message}`); + throw error; + } finally { + // Clean up signal handlers + process.off('SIGINT', cleanup); + process.off('SIGTERM', cleanup); + } }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
api/src/unraid-api/cli/validate-token.command.ts (1)
64-64:⚠️ Potential issueFix potential null reference issue.
Using non-null assertion operator (
!) without proper null checks could lead to runtime errors.- const username = tokenPayload!.username ?? tokenPayload!['cognito:username']; + if (!tokenPayload) { + this.logger.error(createJsonErrorString('Token payload is null')); + return; + } + const username = tokenPayload.username ?? tokenPayload['cognito:username'];api/src/unraid-api/cli/switch-env.command.ts (1)
77-78:⚠️ Potential issueAdd error handling for file operations.
File operations should include proper error handling for robustness.
const newEnvLine = `env="${newEnv}"`; this.logger.debug('Writing %s to %s', newEnvLine, envFlashFilePath); - await writeFile(envFlashFilePath, newEnvLine); + try { + await writeFile(envFlashFilePath, newEnvLine); + } catch (error) { + this.logger.error(`Failed to write to ${envFlashFilePath}:`, error); + throw error; + } // Copy the new env over to live location before restarting const source = join(basePath, `.env.${newEnv}`); const destination = join(basePath, '.env'); cliLogger.debug('Copying %s to %s', source, destination); - await copyFile(source, destination); + try { + await copyFile(source, destination); + } catch (error) { + this.logger.error(`Failed to copy from ${source} to ${destination}:`, error); + throw error; + }Also applies to: 84-85
api/src/unraid-api/cli/key.command.ts (2)
71-71: 🛠️ Refactor suggestionUse dependency injection for ApiKeyService
Direct instantiation of services bypasses NestJS's dependency injection system.
- const apiKeyService = new ApiKeyService();Inject the service in the constructor:
constructor( private readonly logger: Logger, private readonly apiKeyService: ApiKeyService ) { super(); }
69-69: 🛠️ Refactor suggestionReplace console.log with logger
Using console.log is discouraged in production code. Use the injected logger instead.
- console.log(options, passedParams); + this.logger.debug('Command options and parameters:', { options, passedParams });api/src/unraid-api/cli/report.command.ts (1)
364-364: 🛠️ Refactor suggestionReplace console.log with logger
Using console.log is discouraged in production code. Use the injected logger instead.
- console.log({ error }); + this.logger.error('Error generating report:', { error });
🧹 Nitpick comments (7)
api/src/cli.ts (1)
Line range hint
23-28: Enhance error logging for better debugging.The error handling could be more informative by including the error details in the internal logger.
} catch (error) { cliLogger.error('ERROR:', error); internalLogger.error({ message: 'Failed to start unraid-api', - error, + error: error instanceof Error ? { + name: error.name, + message: error.message, + stack: error.stack + } : error, }); process.exit(1); }api/README.md (3)
46-48: Add language identifiers to code blocks.Code blocks should specify the language for proper syntax highlighting.
-``` +```bash unraid-api key get --name "my-app-key"Apply this to all code blocks in the file. Also applies to: 52-54, 58-60, 64-66, 72-74, 78-80, 84-86 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 46-46: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `70-70`: **Add missing article in sentence.** Add "the" before "mothership" for proper grammar. ```diff -To view the current status of the unraid-api and its connection to mothership, run: +To view the current status of the unraid-api and its connection to the mothership, run:🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: You might be missing the article “the” here.
Context: ...of the unraid-api and its connection to mothership, run:unraid-api reportTo vi...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
90-90: Add missing comma in sentence.Add a comma before "please" in the sentence.
-If you'd like to know more about the API and when it's available please join [our discord](https://discord.unraid.net/). +If you'd like to know more about the API and when it's available, please join [our discord](https://discord.unraid.net/).🧰 Tools
🪛 LanguageTool
[typographical] ~90-~90: It seems that a comma is missing.
Context: ...re about the API and when it's available please join [our discord](https://discord.unra...(IF_PLEASE_COMMA)
api/src/unraid-api/cli/key.command.ts (1)
79-88: Improve error handling consistencyConsider standardizing error handling by:
- Using process.exit(1) consistently for all error paths
- Providing more detailed error messages
if (!options) { this.logger.error('Invalid Options for Create Flag'); - return; + process.exit(1); } if (options.roles?.length === 0 && options.permissions?.length === 0) { this.logger.error( 'Please add at least one role or permission with --roles or --permissions' ); - return; + process.exit(1); }api/src/unraid-api/cli/report.command.ts (2)
237-237: Replace void with undefined in union typeUsing void in a union type can be confusing. Use undefined instead for better type clarity.
- async report(options?: ReportOptions): Promise<string | ReportObject | void> { + async report(options?: ReportOptions): Promise<string | ReportObject | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 237-237: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
373-375: Add error handling to run methodThe run method should handle any errors from the report method and ensure proper process exit codes.
async run(_: string[], options?: ReportOptions): Promise<void> { - await this.report(options); + try { + await this.report(options); + } catch (error) { + this.logger.error('Failed to run report:', error); + process.exit(1); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
api/README.md(1 hunks)api/docker-compose.yml(0 hunks)api/package.json(2 hunks)api/src/__test__/cli/commands/report/anonymise-origins.test.ts(0 hunks)api/src/__test__/cli/log-to-syslog.test.ts(0 hunks)api/src/cli.ts(1 hunks)api/src/cli/commands/no-op.ts(0 hunks)api/src/cli/commands/report.ts(0 hunks)api/src/cli/commands/restart.ts(0 hunks)api/src/cli/commands/start.ts(0 hunks)api/src/cli/commands/status.ts(0 hunks)api/src/cli/commands/stop.ts(0 hunks)api/src/cli/commands/switch-env.ts(0 hunks)api/src/cli/commands/validate-token.ts(0 hunks)api/src/cli/commands/version.ts(0 hunks)api/src/cli/index.ts(0 hunks)api/src/cli/log-to-syslog.ts(0 hunks)api/src/cli/options.ts(0 hunks)api/src/cli/set-env.ts(0 hunks)api/src/consts.ts(2 hunks)api/src/core/log.ts(0 hunks)api/src/unraid-api/cli/cli.module.ts(1 hunks)api/src/unraid-api/cli/key.command.ts(1 hunks)api/src/unraid-api/cli/log.service.ts(1 hunks)api/src/unraid-api/cli/logs.command.ts(1 hunks)api/src/unraid-api/cli/report.command.ts(1 hunks)api/src/unraid-api/cli/report.spec.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/status.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/cli/switch-env.command.ts(1 hunks)api/src/unraid-api/cli/validate-token.command.ts(1 hunks)api/src/unraid-api/cli/version.command.ts(1 hunks)api/src/unraid-api/rest/rest.service.ts(2 hunks)
💤 Files with no reviewable changes (17)
- api/src/cli/commands/version.ts
- api/docker-compose.yml
- api/src/cli/commands/stop.ts
- api/src/cli/commands/status.ts
- api/src/cli/set-env.ts
- api/src/test/cli/log-to-syslog.test.ts
- api/src/cli/commands/no-op.ts
- api/src/cli/commands/start.ts
- api/src/cli/log-to-syslog.ts
- api/src/cli/index.ts
- api/src/core/log.ts
- api/src/cli/commands/restart.ts
- api/src/test/cli/commands/report/anonymise-origins.test.ts
- api/src/cli/commands/switch-env.ts
- api/src/cli/commands/validate-token.ts
- api/src/cli/options.ts
- api/src/cli/commands/report.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- api/src/unraid-api/cli/version.command.ts
- api/src/consts.ts
- api/src/unraid-api/cli/report.spec.ts
- api/package.json
- api/src/unraid-api/cli/stop.command.ts
- api/src/unraid-api/cli/cli.module.ts
- api/src/unraid-api/cli/status.command.ts
- api/src/unraid-api/cli/restart.command.ts
- api/src/unraid-api/cli/log.service.ts
- api/src/unraid-api/rest/rest.service.ts
- api/src/unraid-api/cli/start.command.ts
🧰 Additional context used
🪛 LanguageTool
api/README.md
[uncategorized] ~70-~70: You might be missing the article “the” here.
Context: ...of the unraid-api and its connection to mothership, run: unraid-api report To vi...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~90-~90: It seems that a comma is missing.
Context: ...re about the API and when it's available please join [our discord](https://discord.unra...
(IF_PLEASE_COMMA)
🪛 Markdownlint (0.37.0)
api/README.md
46-46: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
52-52: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
64-64: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
72-72: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
78-78: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
84-84: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Biome (1.9.4)
api/src/unraid-api/cli/report.command.ts
[error] 237-237: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
api/src/unraid-api/cli/logs.command.ts
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
api/src/cli.ts (2)
19-19:⚠️ Potential issueTrim shell path to prevent potential issues.
The shell path from
execSyncmight contain trailing newlines which could cause issues.- nativeShell: { executablePath: shellToUse.toString('utf-8') }, + nativeShell: { executablePath: shellToUse.toString('utf-8').trim() },Likely invalid or redundant comment.
12-12:⚠️ Potential issueAdd error handling for shell path determination.
The
execSynccall could throw ifbashis not available. Add proper error handling to provide a fallback shell or meaningful error message.- const shellToUse = execSync('which bash'); + let shellToUse; + try { + shellToUse = execSync('which bash'); + } catch (error) { + try { + shellToUse = execSync('which sh'); + } catch (error) { + cliLogger.error('No suitable shell found. Please ensure bash or sh is available.'); + process.exit(1); + } + }Likely invalid or redundant comment.
api/src/unraid-api/cli/switch-env.command.ts (1)
87-87:⚠️ Potential issueAdd error handling for start command.
The start command execution should include error handling.
- await this.startCommand.run(null, {}); + try { + await this.startCommand.run(null, {}); + } catch (error) { + this.logger.error('Failed to start the Unraid API:', error); + throw error; + }Likely invalid or redundant comment.
api/src/unraid-api/cli/key.command.ts (4)
24-26: Verify the parseCreate implementationThe method always returns true regardless of input, which might not be the intended behavior. Consider if this should actually parse a boolean value from the command line argument.
32-50: LGTM! Well-implemented role validation.The implementation efficiently validates roles using Set, provides clear error messages, and properly handles invalid roles by warning the user.
64-66: 🛠️ Refactor suggestionHandle unimplemented permissions gracefully
Throwing an error in an option parser will cause the entire CLI command to fail. Consider returning null or an empty array with a warning message until the permissions feature is implemented.
- parsePermissions(permissions: string) { - throw new Error('Stub Method Until Permissions PR is merged'); + parsePermissions(permissions: string): string[] | null { + this.logger.warn('Permissions feature is not yet implemented'); + return null; + }Likely invalid or redundant comment.
18-18: 🛠️ Refactor suggestionUse dependency injection for Logger
Direct instantiation of services bypasses NestJS's dependency injection. Inject Logger through the constructor instead, similar to how it's done in report.command.ts.
- private readonly logger = new Logger(KeyCommand.name); + constructor(private readonly logger: Logger) { + super(); + }Likely invalid or redundant comment.
api/src/unraid-api/cli/report.command.ts (1)
17-54: LGTM! Well-structured types and utility functions.The implementation demonstrates good TypeScript practices with:
- Clear and comprehensive type definitions
- Well-organized utility functions
- Strong typing for all components
Also applies to: 105-131
| const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); | ||
|
|
||
| subprocess.stdout?.on('data', (data) => { | ||
| this.logger.log(data.toString()); | ||
| }); | ||
|
|
||
| subprocess.stderr?.on('data', (data) => { | ||
| this.logger.error(data.toString()); | ||
| }); | ||
|
|
||
| await subprocess; |
There was a problem hiding this comment.
Add error handling for subprocess execution.
The subprocess execution should handle potential failures and cleanup resources.
- const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]);
+ try {
+ const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]);
+
+ subprocess.stdout?.on('data', (data) => {
+ this.logger.log(data.toString());
+ });
+
+ subprocess.stderr?.on('data', (data) => {
+ this.logger.error(data.toString());
+ });
+
+ await subprocess;
+ } catch (error) {
+ this.logger.error(`Failed to execute PM2 logs command: ${error instanceof Error ? error.message : 'Unknown error'}`);
+ }
- subprocess.stdout?.on('data', (data) => {
- this.logger.log(data.toString());
- });
-
- subprocess.stderr?.on('data', (data) => {
- this.logger.error(data.toString());
- });
-
- await subprocess;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); | |
| subprocess.stdout?.on('data', (data) => { | |
| this.logger.log(data.toString()); | |
| }); | |
| subprocess.stderr?.on('data', (data) => { | |
| this.logger.error(data.toString()); | |
| }); | |
| await subprocess; | |
| try { | |
| const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); | |
| subprocess.stdout?.on('data', (data) => { | |
| this.logger.log(data.toString()); | |
| }); | |
| subprocess.stderr?.on('data', (data) => { | |
| this.logger.error(data.toString()); | |
| }); | |
| await subprocess; | |
| } catch (error) { | |
| this.logger.error(`Failed to execute PM2 logs command: ${error instanceof Error ? error.message : 'Unknown error'}`); | |
| } |
| const lines = options?.lines ?? 100; | ||
| const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for negative line count.
The line count should be validated to prevent negative values.
const lines = options?.lines ?? 100;
+ if (lines <= 0) {
+ this.logger.error('Line count must be positive');
+ return;
+ }
const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]);Committable suggestion skipped: line range outside the PR's diff.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 30
🔭 Outside diff range comments (2)
api/src/__test__/store/modules/config.test.ts (1)
Line range hint
19-47: Avoid hardcoding sensitive data in tests.The test cases contain hardcoded API keys and tokens. Consider:
- Using environment variables or test fixtures
- Using placeholder values that clearly indicate test data
- apikey: '_______________________BIG_API_KEY_HERE_________________________', - localApiKey: '_______________________LOCAL_API_KEY_HERE_________________________', + apikey: process.env.TEST_API_KEY || 'test_api_key', + localApiKey: process.env.TEST_LOCAL_API_KEY || 'test_local_api_key',Also applies to: 67-97
api/src/cli.ts (1)
Line range hint
23-28: Enhance error handling with specific error types.The error handling is too generic and logs the entire error object.
Improve error handling:
} catch (error) { - cliLogger.error('ERROR:', error); + if (error instanceof Error) { + cliLogger.error('ERROR:', error.message); + if (error.stack) { + internalLogger.debug(error.stack); + } + } else { + cliLogger.error('An unexpected error occurred'); + } internalLogger.error({ message: 'Failed to start unraid-api', - error, + error: error instanceof Error ? error.message : String(error), }); process.exit(1); }
♻️ Duplicate comments (1)
api/src/cli.ts (1)
12-12:⚠️ Potential issueEnsure error handling for the
execSynccommand.The code uses
execSync('which bash')to determine the shell path but does not handle the case wherebashmay not be available.
🧹 Nitpick comments (23)
api/src/__test__/core/utils/images/image-file-helpers.test.ts (2)
11-12: Fix spacing in await statement.Remove the extra space between
awaitandexpectfor consistent formatting.- await expect(getBannerPathIfPresent()).resolves.toMatchInlineSnapshot('null') + await expect(getBannerPathIfPresent()).resolves.toMatchInlineSnapshot('null')
7-22: Consider adding tests for edge cases.While the current test coverage is good, consider adding tests for:
- Invalid path handling
- File system errors
- Permission issues
This would improve the robustness of the test suite.
api/src/__test__/common/allowed-origins.test.ts (1)
27-28: Consider using DNS names instead of IP addresses.Using hardcoded IP addresses ("192.168.1.150") in allowed origins can be problematic if the server's IP changes. Consider using DNS names consistently instead.
api/src/core/utils/files/config-file-normalizer.ts (1)
1-1: Remove unused import of 'isEqual'The import of
isEqualfrom'lodash-es'at line 1 is no longer used since theareConfigsEquivalentfunction has been removed. Removing unused imports improves code cleanliness and reduces potential confusion.Apply this diff to remove the unused import:
-import { isEqual } from 'lodash-es';api/src/core/sso/sso-setup.ts (1)
24-24: Replace magic number with a named constant for clarityUsing
800as a threshold for password length without explanation can make the code less readable and maintainable. Defining a named constant with a descriptive name enhances clarity.Apply this diff to introduce a named constant:
+ // Define threshold length to distinguish between passwords and SSO tokens + $SSO_TOKEN_LENGTH_THRESHOLD = 800; // We may have an SSO token, attempt validation - if (strlen($password) > 800) { + if (strlen($password) > $SSO_TOKEN_LENGTH_THRESHOLD) { $safePassword = escapeshellarg($password);api/src/unraid-api/cli/sso/validate-token.command.ts (1)
83-85: Ensure output format matches command descriptionThe command description specifies the output as
JSON: { error: string | null, valid: boolean }, but the successful response includes an additionalusernamefield. For consistency, update the description or adjust the output.Option 1: Update the command description to include
username:@SubCommand({ name: 'validate-token', aliases: ['validate', 'v'], - description: 'Returns JSON: { error: string | null, valid: boolean }', + description: 'Returns JSON: { error: string | null, valid: boolean, username: string }', arguments: '<token>', })Option 2: Remove
usernamefrom the output:if (possibleUserIds.includes(username)) { - this.logger.info(JSON.stringify({ error: null, valid: true, username })); + this.logger.info(JSON.stringify({ error: null, valid: true })); process.exit(0); } else {web/components/SsoButton.ce.vue (2)
37-37: Avoid hardcoding the usernameHardcoding the username to
'root'limits flexibility and may pose security risks. Consider making the username dynamic or configurable to accommodate different users.
84-84: Externalizeclient_idto configurationHardcoding
client_id('CONNECT_SERVER_SSO') reduces flexibility and may expose sensitive information. Consider movingclient_idto a configuration file or environment variable for better security practices.api/src/core/sso/sso-remove.ts (1)
6-7: Consider extracting hardcoded paths to configuration.The hardcoded paths should be moved to a configuration file or environment variables for better maintainability and flexibility.
+import { config } from '@app/config'; + export const removeSso = () => { - const path = '/usr/local/emhttp/plugins/dynamix/include/.login.php'; - const backupPath = path + '.bak'; + const path = config.sso.loginFilePath; + const backupPath = `${path}.bak`;api/src/unraid-api/cli/sso/remove-sso-user.questions.ts (1)
23-26: Consider using a more descriptive error message and non-zero exit code.When no SSO users are found, the error message could be more helpful, and exiting with code 0 suggests success rather than an error condition.
- this.logger.error('No SSO Users Found'); - process.exit(0); + this.logger.error('No SSO users found. Please add users first using the add-sso-user command.'); + process.exit(1);api/src/unraid-api/cli/sso/sso.command.ts (1)
22-25: Consider providing command usage examples in the help message.The current implementation only shows a basic help message. Adding examples would improve user experience.
async run(): Promise<void> { - this.logger.info('Please provide a subcommand or use --help for more information'); + this.logger.info(` +Please provide a subcommand or use --help for more information + +Examples: + sso add-user # Add a new SSO user + sso remove-user # Remove an SSO user + sso list-users # List all SSO users + sso validate-token # Validate an SSO token +`); process.exit(0); }api/src/core/sso/auth-request-setup.ts (2)
9-10: Consider making file paths configurable.Hardcoded paths could cause issues in different environments or directory structures.
Consider moving these paths to environment variables or configuration:
-const AUTH_REQUEST_FILE = '/usr/local/emhttp/auth-request.php'; -const WEB_COMPS_DIR = '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/'; +const AUTH_REQUEST_FILE = process.env.AUTH_REQUEST_FILE || '/usr/local/emhttp/auth-request.php'; +const WEB_COMPS_DIR = process.env.WEB_COMPS_DIR || '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/';
27-29: Enhance backup mechanism.Current backup mechanism could overwrite existing backups without warning.
Consider adding timestamp to backup files and limiting the number of backups:
- const backupFile = `${AUTH_REQUEST_FILE}.bak`; + const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + const backupFile = `${AUTH_REQUEST_FILE}.${timestamp}.bak`;api/src/unraid-api/cli/sso/add-sso-user.questions.ts (1)
43-45: Move UUID validation to a shared utility.UUID validation is duplicated across files and could be centralized.
Create a shared validation utility:
// shared/validators.ts export const UUID_REGEX = /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/; export const validateUUID = (input: string): boolean => UUID_REGEX.test(input);api/src/types/my-servers-config.ts (1)
25-41: Improve error handling for ssoSubIds validation.The current implementation silently replaces invalid values with an empty string. Consider:
- Throwing an error instead of silent replacement
- Adding proper validation for individual IDs
ssoSubIds: z .string() - .transform((val) => { - // If valid, return as is - if (val === '' || val.split(',').every((id) => id.trim().match(/^[a-zA-Z0-9-]+$/))) { - return val; - } - // Otherwise, replace with an empty string - return ''; - }) .refine( (val) => val === '' || val.split(',').every((id) => id.trim().match(/^[a-zA-Z0-9-]+$/)), { message: 'ssoSubIds must be empty or a comma-separated list of alphanumeric strings with dashes', } ),api/src/unraid-api/cli/sso/add-sso-user.command.ts (1)
57-57: Fix typos in error messages.There are typos in the error messages:
- Template literal syntax is incorrect in UUID error message
- "diclaimer" is misspelled
- throw new Error('Username must be in the format of a UUID (e.g., ${v4()}}\n'); + throw new Error(`Username must be in the format of a UUID (e.g., ${v4()})\n`); // ... - throw new Error('Please answer the diclaimer with (y/n)\n'); + throw new Error('Please answer the disclaimer with (y/n)\n');Also applies to: 69-69
web/pages/webComponents.vue (1)
78-83: Improve comment style and type safety.
- The HTML comment style could be improved for better maintainability
- Consider adding type safety for the serverState prop
- <!-- uncomment to test modals <unraid-modals />--> + <!-- TODO: Uncomment to test modals --> + <!-- <unraid-modals /> --> <hr class="border-black dark:border-white"> <h3 class="text-lg font-semibold font-mono"> SSOSignInButtonCe </h3> - <unraid-sso-button :ssoenabled="serverState.ssoEnabled" /> + <unraid-sso-button :ssoenabled="Boolean(serverState.ssoEnabled)" />api/src/__test__/store/modules/config.test.ts (1)
7-9: Consider reducing test timeout.The 10-second timeout seems excessive for a snapshot test. Consider reducing it to a more reasonable value.
test('Before init returns default values for all fields', async () => { const state = store.getState().config; expect(state).toMatchSnapshot(); -}, 10_000); +}, 1_000);web/pages/index.vue (1)
156-160: Follow Vue prop naming convention.The prop name should be camelCase in Vue. Change
:ssoenabledto:ssoEnabledto follow Vue's style guide.- <SsoButtonCe :ssoenabled="serverState.ssoEnabled" /> + <SsoButtonCe :ssoEnabled="serverState.ssoEnabled" />web/store/server.ts (2)
158-158: Add type annotation forssoEnabledref.The
ssoEnabledref lacks explicit type annotation.Add type annotation:
-const ssoEnabled = ref<boolean>(false); +const ssoEnabled = ref<boolean>(false); + +// Add type to Server interface if not already present +interface Server { + // ...existing properties + ssoEnabled?: boolean; +}
1212-1214: Consider using nullish coalescing for SSO state.The current implementation uses
undefinedcheck which might not handle all falsy values correctly.Use nullish coalescing for more precise handling:
-if (typeof data?.ssoEnabled !== "undefined") { - ssoEnabled.value = Boolean(data.ssoEnabled); -} +ssoEnabled.value = Boolean(data?.ssoEnabled ?? false);web/_data/serverState.ts (2)
174-174: Simplify boolean expression.The conditional expression can be simplified.
- registered: connectPluginInstalled ? true : false, + registered: !!connectPluginInstalled,🧰 Tools
🪛 Biome (1.9.4)
[error] 174-174: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
47-50: Consider extracting mock data configuration.The file contains a significant amount of mock/test data mixed with the actual state definition. Consider extracting this into a separate mock data configuration file for better maintainability.
Also applies to: 68-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/package-lock.jsonis excluded by!**/package-lock.jsonapi/src/__test__/store/modules/__snapshots__/config.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (44)
.github/workflows/main.yml(1 hunks)api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/states/myservers.cfg(1 hunks)api/package.json(3 hunks)api/src/__test__/common/allowed-origins.test.ts(1 hunks)api/src/__test__/core/utils/files/config-file-normalizer.test.ts(3 hunks)api/src/__test__/core/utils/images/image-file-helpers.test.ts(1 hunks)api/src/__test__/store/modules/config.test.ts(3 hunks)api/src/cli.ts(1 hunks)api/src/consts.ts(3 hunks)api/src/core/sso/auth-request-setup.ts(1 hunks)api/src/core/sso/sso-remove.ts(1 hunks)api/src/core/sso/sso-setup.ts(1 hunks)api/src/core/utils/files/config-file-normalizer.ts(1 hunks)api/src/index.ts(2 hunks)api/src/store/listeners/config-listener.ts(1 hunks)api/src/store/modules/config.ts(8 hunks)api/src/store/watch/config-watch.ts(1 hunks)api/src/types/my-servers-config.d.ts(0 hunks)api/src/types/my-servers-config.ts(1 hunks)api/src/unraid-api/auth/header.strategy.ts(1 hunks)api/src/unraid-api/cli/cli.module.ts(1 hunks)api/src/unraid-api/cli/config.command.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(1 hunks)api/src/unraid-api/cli/sso/add-sso-user.command.ts(1 hunks)api/src/unraid-api/cli/sso/add-sso-user.questions.ts(1 hunks)api/src/unraid-api/cli/sso/list-sso-user.command.ts(1 hunks)api/src/unraid-api/cli/sso/remove-sso-user.command.ts(1 hunks)api/src/unraid-api/cli/sso/remove-sso-user.questions.ts(1 hunks)api/src/unraid-api/cli/sso/sso.command.ts(1 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)plugin/plugins/dynamix.unraid.net.plg(0 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/sso-login.php(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php(12 hunks)web/_data/serverState.ts(6 hunks)web/components/SsoButton.ce.vue(1 hunks)web/nuxt.config.ts(1 hunks)web/pages/index.vue(2 hunks)web/pages/login.vue(1 hunks)web/pages/webComponents.vue(1 hunks)web/store/server.ts(3 hunks)web/types/server.ts(3 hunks)
💤 Files with no reviewable changes (2)
- plugin/plugins/dynamix.unraid.net.plg
- api/src/types/my-servers-config.d.ts
✅ Files skipped from review due to trivial changes (1)
- api/src/unraid-api/auth/header.strategy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/consts.ts
- api/src/unraid-api/cli/stop.command.ts
- api/src/unraid-api/cli/start.command.ts
- api/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/_data/serverState.ts
[error] 174-174: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🪛 Gitleaks (8.21.2)
web/_data/serverState.ts
149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (19)
api/src/__test__/core/utils/images/image-file-helpers.test.ts (3)
7-8: LGTM! Proper async/await handling added.The test correctly uses async/await with Promise resolution, improving the reliability of the test.
17-17: LGTM! Proper async handling with state loading.The test correctly awaits both the state loading and the assertion, ensuring reliable test execution.
22-22: LGTM! Good negative test case handling.The test properly handles the negative case scenario with correct async/await usage.
api/src/__test__/common/allowed-origins.test.ts (2)
21-46: LGTM! Clean snapshot format.The new array format for the snapshot is more readable and maintainable than a multiline string.
41-42:⚠️ Potential issueReview overly permissive allowed origins.
The inclusion of generic domains like "google.com" and "test.com" in the allowed origins list poses a security risk. These should be removed unless there's a specific requirement for them.
Let's verify if these origins are actually required by checking their usage:
api/package.json (2)
26-26: LGTM: New command script follows best practices.The new command script ensures the latest code is built before running the CLI and improves output readability by clearing the terminal.
84-84: Architectural improvement: Migration to nest-commander.The shift from
ts-command-line-argstonest-commanderis a good architectural decision that:
- Provides better integration with the NestJS ecosystem
- Offers improved type safety and validation
- Enables more structured command handling through decorators
- Aligns with NestJS best practices
The addition of
globis appropriate for file pattern matching needs in the new CLI implementation.To ensure smooth migration, verify that:
- All existing CLI functionality has been migrated
- Command help documentation is updated
- Breaking changes are documented in README
Also applies to: 104-104
✅ Verification successful
CLI migration to nest-commander successfully verified
The migration has been completed thoroughly:
- All CLI functionality has been properly migrated using nest-commander decorators and patterns
- Command help documentation is up-to-date in README.md
- No remaining ts-command-line-args usage in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CLI migration completeness # Check for any remaining ts-command-line-args usage echo "Checking for remaining ts-command-line-args usage..." rg "ts-command-line-args" # Look for CLI-related files and their content echo -e "\nListing CLI-related files..." fd -e ts -e js cli # Check for command documentation echo -e "\nChecking for command documentation..." fd "README|CHANGELOG" -t fLength of output: 985
Script:
#!/bin/bash # Check nest-commander usage and CLI implementation echo "Checking nest-commander usage..." rg "nest-commander" echo -e "\nExamining CLI implementation..." cat api/src/cli.ts cat api/src/unraid-api/cli/cli.module.ts echo -e "\nChecking documentation..." cat api/README.md cat api/CHANGELOG.md | grep -A 5 "BREAKING CHANGE"Length of output: 8161
api/src/core/sso/sso-setup.ts (2)
27-27: Avoid logging potentially sensitive informationLogging the
$responsefrom the SSO validation might expose sensitive information. To protect user data and adhere to security best practices, consider logging only necessary information.[security]
Apply this diff to modify the log statement:
- my_logger("SSO Login Response: $response"); + my_logger("SSO login attempt: " . ($code === 0 ? "successful" : "failed"));
59-61: Ensure robust HTML modification when injecting PHP includeIf the
</form>tag does not exist in the target file, the replacement will fail silently, possibly leading to incomplete SSO setup. Consider adding a check to confirm that the</form>tag is present or handle cases where it is absent.Apply this diff to add a verification step:
// Inject the PHP include tag after the closing </form> tag - fileContent = fileContent.replace(/<\/form>/i, `</form>\n${tagToInject}`); + const formTagPattern = /<\/form>/i; + if (formTagPattern.test(fileContent)) { + fileContent = fileContent.replace(formTagPattern, `</form>\n${tagToInject}`); + } else { + // Handle the absence of </form> tag appropriately + this.logger.error('Could not find </form> tag to inject SSO include.'); + // Optionally, decide whether to proceed or abort + return; + }api/src/unraid-api/cli/sso/validate-token.command.ts (1)
61-61: Handle possible sensitive information in error messagesWhen catching errors during JWT validation, the error message may contain sensitive details. Ensure that error logging does not expose any sensitive information.
[security]
Apply this diff to redact sensitive details:
this.createErrorAndExit(`Caught error validating JWT token: An error occurred during token validation.`);api/dev/Unraid.net/myservers.cfg (1)
19-19: Configuration updates align with code changesThe addition of
ssoSubIdsin the[remote]section is consistent with the code changes and supports the new SSO functionality.api/src/unraid-api/cli/sso/remove-sso-user.questions.ts (1)
22-22: Add input validation for malformed ssoSubIds.The split operation assumes well-formed input. Consider adding validation to handle potential malformed data.
- const users = store.getState().config.remote.ssoSubIds.split(',').filter((user) => user !== ''); + const ssoSubIds = store.getState().config.remote.ssoSubIds; + if (typeof ssoSubIds !== 'string') { + this.logger.error('Invalid ssoSubIds format in config'); + process.exit(1); + } + const users = ssoSubIds.split(',').filter((user) => user !== '');api/dev/states/myservers.cfg (1)
20-20: Review and validate the allowed origins list.The
allowedOriginslist contains many development and test URLs. Consider separating development and production configurations.web/nuxt.config.ts (1)
152-154: LGTM! Clean addition of SSO button component.The UnraidSsoButton component is properly integrated into the customElements configuration, following the established naming and path conventions.
api/src/__test__/core/utils/files/config-file-normalizer.test.ts (1)
29-30: LGTM! Comprehensive test coverage for new SSO properties.The test snapshots have been properly updated to include the new SSO-related properties (ssoSubIds) in both FLASH and MEMORY configurations, with proper handling of optional and non-optional cases.
Also applies to: 64-65, 102-103, 144-145
web/types/server.ts (1)
112-112: LGTM! Clean type definition for SSO feature flag.The ssoEnabled property is properly typed as an optional boolean, following TypeScript best practices.
web/pages/index.vue (1)
7-7: LGTM! Clean import of SSO button component.The SsoButtonCe component is properly imported following Vue conventions.
api/src/unraid-api/cli/cli.module.ts (1)
24-45: LGTM! Well-organized module structure.The module follows NestJS best practices with a clean organization of commands by functionality (SSO, logging, configuration, etc.).
.github/workflows/main.yml (1)
245-245: Verify the removal of base branch restriction.The condition change allows PR builds for any branch, not just those targeting 'main'. While this enables testing on feature branches, it could increase resource usage.
Please confirm if this change is intentional and aligns with the team's branching strategy.
| /** | ||
| * SSO Sub IDs from the my servers config file. | ||
| */ | ||
| public $ssoEnabled = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the SSO property private with a getter.
The $ssoEnabled property is declared as public, which violates encapsulation principles.
Make the property private and add a getter method:
-/**
- * SSO Sub IDs from the my servers config file.
- */
-public $ssoEnabled = false;
+/**
+ * SSO Sub IDs from the my servers config file.
+ * @var bool
+ */
+private $ssoEnabled = false;
+
+/**
+ * Get the SSO enabled state
+ * @return bool
+ */
+public function isSsoEnabled(): bool
+{
+ return $this->ssoEnabled;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * SSO Sub IDs from the my servers config file. | |
| */ | |
| public $ssoEnabled = false; | |
| /** | |
| * SSO Sub IDs from the my servers config file. | |
| * @var bool | |
| */ | |
| private $ssoEnabled = false; | |
| /** | |
| * Get the SSO enabled state | |
| * @return bool | |
| */ | |
| public function isSsoEnabled(): bool | |
| { | |
| return $this->ssoEnabled; | |
| } |
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/cli.ts (2)
22-22: Remove redundant process exit.The
process.exit(0)is unnecessary as the process would naturally exit after successful completion.- process.exit(0);
24-24: Improve error message formatting for CLI output.The error object should be formatted appropriately for CLI display:
- cliLogger.error('ERROR:', error); + cliLogger.error('ERROR:', error.message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/cli.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/cli.ts (3)
4-9: LGTM! Well-organized imports.The imports are properly organized, separating external dependencies from internal modules.
13-21: Consider environment compatibility for top-level await.The use of top-level await requires Node.js 14+ and ECMAScript modules.
#!/bin/bash # Check the minimum Node.js version requirement in package.json jq '.engines.node' package.json
12-12: 🛠️ Refactor suggestionAdd encoding and validation for shell path determination.
The shell path determination needs proper encoding and validation:
-const shellToUse = execSync('which bash').toString().trim(); +const shellToUse = execSync('which bash').toString('utf-8').trim(); +if (!shellToUse) { + throw new Error('Bash shell not found'); +}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
web/_data/serverState.ts (3)
Line range hint
1-13: Clean up commented import statements.Consider removing the commented-out import statements if they are no longer needed. If they might be needed in the future, add a TODO comment explaining why they are kept.
68-103: Enhance state management code maintainability.
- Consider extracting magic numbers for device counts into named constants
- Add a default case to the switch statement to handle unexpected states
+const BASIC_DEVICE_LIMIT = 6; +const PLUS_DEVICE_LIMIT = 12; +const UNLIMITED_DEVICES = -1; switch (state) { case 'BASIC': - regDevs = 6; + regDevs = BASIC_DEVICE_LIMIT; regTy = 'Basic'; break; case 'PLUS': - regDevs = 12; + regDevs = PLUS_DEVICE_LIMIT; regTy = 'Plus'; break; case 'PRO': - regDevs = -1; + regDevs = UNLIMITED_DEVICES; regTy = 'Pro'; break; + default: + console.warn(`Unexpected server state: ${state}`); + break;
173-173: Simplify boolean expression.The boolean expression can be simplified by directly using the value.
- registered: connectPluginInstalled ? true : false, + registered: !!connectPluginInstalled,🧰 Tools
🪛 Biome (1.9.4)
[error] 173-173: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
web/components/SsoButton.ce.vue (2)
5-8: Consider consolidating similar props.Having two props
ssoenabledandssoEnabledwith different casing is confusing. Consider deprecating one and using a single consistent prop name.export interface Props { - ssoenabled?: boolean | string; - ssoEnabled?: boolean; + ssoEnabled?: boolean | string; }
101-106: Improve error handling specificity.The current error handling catches all errors and displays a generic message. Consider providing more specific error messages based on the error type.
} catch (err) { console.error('Error fetching token', err); + const errorMessage = err instanceof Error ? err.message : 'Error fetching token'; currentState.value = 'error'; - error.value = 'Error fetching token'; + error.value = errorMessage; reEnableFormOnError(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php(12 hunks)web/_data/serverState.ts(6 hunks)web/_webGui/testWebComponents.page(0 hunks)web/components/DownloadApiLogs.ce.vue(1 hunks)web/components/SsoButton.ce.vue(1 hunks)web/helpers/create-apollo-client.ts(0 hunks)web/store/server.ts(3 hunks)web/types/server.ts(3 hunks)
💤 Files with no reviewable changes (2)
- web/helpers/create-apollo-client.ts
- web/_webGui/testWebComponents.page
🚧 Files skipped from review as they are similar to previous changes (3)
- web/store/server.ts
- web/types/server.ts
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php
🧰 Additional context used
🪛 Biome (1.9.4)
web/_data/serverState.ts
[error] 173-173: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
web/components/SsoButton.ce.vue (2)
48-53: Use a cryptographically secure method to generate state tokens.Generating state tokens using
Math.random()is insecure because it is not cryptographically secure.
134-149: LGTM! Well-structured template with proper error handling.The template follows Vue best practices with clean conditional rendering and proper error state handling.
web/components/DownloadApiLogs.ce.vue (1)
11-11: Security improvement: API key removed from URL.Good change! Removing the API key from the URL prevents potential exposure in logs, browser history, and referrer headers.
| const state: ServerState = 'ENOKEYFILE' as ServerState; | ||
| const currentFlashGuid = '1111-1111-YIJD-ZACK1234TEST'; // this is the flash drive that's been booted from | ||
| const regGuid = '1111-1111-YIJD-ZACK1234TEST'; // this guid is registered in key server | ||
| const keyfileBase64 = ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove test data from production code.
The file contains hard-coded test GUIDs. These should be moved to a test configuration file or environment variables.
-const currentFlashGuid = '1111-1111-YIJD-ZACK1234TEST'; // this is the flash drive that's been booted from
-const regGuid = '1111-1111-YIJD-ZACK1234TEST'; // this guid is registered in key server
+const currentFlashGuid = process.env.FLASH_GUID || '';
+const regGuid = process.env.REG_GUID || '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const state: ServerState = 'ENOKEYFILE' as ServerState; | |
| const currentFlashGuid = '1111-1111-YIJD-ZACK1234TEST'; // this is the flash drive that's been booted from | |
| const regGuid = '1111-1111-YIJD-ZACK1234TEST'; // this guid is registered in key server | |
| const keyfileBase64 = ''; | |
| const state: ServerState = 'ENOKEYFILE' as ServerState; | |
| const currentFlashGuid = process.env.FLASH_GUID || ''; | |
| const regGuid = process.env.REG_GUID || ''; | |
| const keyfileBase64 = ''; |
| code: 'CC2KP3TDRF', | ||
| partnerName: 'OEM Partner', | ||
| partnerUrl: 'https://unraid.net/OEM+Partner', | ||
| sysModel: 'OEM Partner v1', | ||
| comment: 'OEM Partner NAS', | ||
| caseIcon: 'case-model.png', | ||
| header: '#ffffff', | ||
| headermetacolor: '#eeeeee', | ||
| background: '#000000', | ||
| showBannerGradient: 'yes', | ||
| partnerLogo: true, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize configuration values.
Consider moving the activation code data and partner-specific configuration to environment variables or a configuration file. This would make it easier to manage different environments and partner configurations.
+import { config } from '~/config/partners';
export const serverState: Server = {
- activationCodeData: {
- code: 'CC2KP3TDRF',
- partnerName: 'OEM Partner',
- partnerUrl: 'https://unraid.net/OEM+Partner',
- sysModel: 'OEM Partner v1',
- comment: 'OEM Partner NAS',
- caseIcon: 'case-model.png',
- header: '#ffffff',
- headermetacolor: '#eeeeee',
- background: '#000000',
- showBannerGradient: 'yes',
- partnerLogo: true,
- },
+ activationCodeData: config.partner,Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
These serverState values are used for testing.
There was a problem hiding this comment.
@elibosley Ah, I understand. Thank you for the clarification. I'll retract my suggestion about externalizing these values since they are meant for testing purposes.
✏️ Learnings added
Learnt from: elibosley
PR: unraid/api#1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.409Z
Learning: The values in `web/_data/serverState.ts` are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const navigateToExternalSSOUrl = () => { | ||
| const url = new URL('sso', ACCOUNT); | ||
| const callbackUrlLogin = new URL('login', window.location.origin); | ||
| const state = generateStateToken(); | ||
| url.searchParams.append('callbackUrl', callbackUrlLogin.toString()); | ||
| url.searchParams.append('state', state); | ||
| window.location.href = url.toString(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add origin URL validation.
The current implementation doesn't validate the origin URL before navigation. Consider adding validation to prevent potential security issues.
const navigateToExternalSSOUrl = () => {
+ const allowedOrigins = ['http://localhost', 'https://unraid.net'];
+ if (!allowedOrigins.some(origin => window.location.origin.startsWith(origin))) {
+ console.error('Invalid origin');
+ return;
+ }
const url = new URL('sso', ACCOUNT);
const callbackUrlLogin = new URL('login', window.location.origin);
const state = generateStateToken();
url.searchParams.append('callbackUrl', callbackUrlLogin.toString());
url.searchParams.append('state', state);
window.location.href = url.toString();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const navigateToExternalSSOUrl = () => { | |
| const url = new URL('sso', ACCOUNT); | |
| const callbackUrlLogin = new URL('login', window.location.origin); | |
| const state = generateStateToken(); | |
| url.searchParams.append('callbackUrl', callbackUrlLogin.toString()); | |
| url.searchParams.append('state', state); | |
| window.location.href = url.toString(); | |
| }; | |
| const navigateToExternalSSOUrl = () => { | |
| const allowedOrigins = ['http://localhost', 'https://unraid.net']; | |
| if (!allowedOrigins.some(origin => window.location.origin.startsWith(origin))) { | |
| console.error('Invalid origin'); | |
| return; | |
| } | |
| const url = new URL('sso', ACCOUNT); | |
| const callbackUrlLogin = new URL('login', window.location.origin); | |
| const state = generateStateToken(); | |
| url.searchParams.append('callbackUrl', callbackUrlLogin.toString()); | |
| url.searchParams.append('state', state); | |
| window.location.href = url.toString(); | |
| }; |
| const { apiKey } = storeToRefs(useServerStore()); | ||
| const downloadUrl = computed(() => new URL(`/graphql/api/logs?apiKey=${apiKey.value}`, WEBGUI_GRAPHQL)); | ||
| const downloadUrl = computed(() => new URL(`/graphql/api/logs`, WEBGUI_GRAPHQL)); |
There was a problem hiding this comment.
💡 Codebase verification
Authentication mechanism verified, documentation update needed
The logs endpoint is properly secured using permission-based authentication:
- Uses standard auth middleware requiring READ permission on LOGS resource
- No URL-based API key needed (removed as intended)
However, documentation in the playground section needs to be updated to remove references to the old API key authentication method.
🔗 Analysis chain
Verify authentication mechanism for the download endpoint.
Since the API key is no longer passed in the URL, please confirm:
- How is the download endpoint authenticated now?
- Are there any changes needed in the documentation to reflect this authentication change?
Let's verify the authentication mechanism:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related authentication middleware or handlers
rg -l "graphql/api/logs" | xargs rg -A 5 -B 5 "middleware|auth|guard"
# Search for documentation mentioning the logs endpoint
rg -l "graphql/api/logs" | xargs rg -A 5 -B 5 "download.*logs|logs.*download"
Length of output: 1861
Script:
#!/bin/bash
# Search for the logs endpoint implementation
ast-grep --pattern 'class RestController {
$$$
@Get($$$"/graphql/api/logs"$$$)
$$$
}'
# Backup search using ripgrep
rg -A 10 -B 2 "@Get.*graphql/api/logs" api/src/unraid-api/rest/rest.controller.ts
Length of output: 591
Script:
#!/bin/bash
# Search for documentation files
fd -e md -e mdx | xargs rg -l "api.*logs|logs.*api|download.*logs"
# Also check for any API documentation
fd "api|swagger|openapi" -e yaml -e json | xargs rg -l "api.*logs|logs.*api"
Length of output: 500
Script:
#!/bin/bash
# Check documentation content
echo "=== Development Documentation ==="
rg -A 5 -B 5 "api.*logs|logs.*api|authentication|api.?key" api/docs/development.md
echo -e "\n=== Changelog ==="
rg -A 5 -B 5 "api.*logs|logs.*api|authentication|api.?key" api/CHANGELOG.md
Length of output: 31243
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/src/unraid-api/cli/report.command.ts (5)
25-53: Add JSDoc documentation for the ReportObject type.Consider adding JSDoc documentation to describe the purpose and structure of the ReportObject type, as it's a central type that defines the shape of the report output.
+/** + * Represents the structure of a complete API report. + * Contains information about the OS, API status, servers, and cloud connectivity. + */ type ReportObject = {
63-69: Improve parameter naming for better clarity.The parameter
vis not descriptive. Consider renaming it toverbosityfor better code readability.export const getServersData = async ({ client, - v, + verbosity, }: { client: ApolloClient<NormalizedCacheObject>; - v: number; + verbosity: number; }): Promise<ServersPayload | null> => {
106-121: Add detailed comments for regex replacements.The regex replacements in anonymiseOrigins are complex and would benefit from more detailed comments explaining each transformation.
export const anonymiseOrigins = (origins?: string[]): string[] => { const originsWithoutSocks = origins?.filter((url) => !url.endsWith('.sock')) ?? []; return originsWithoutSocks .map((origin) => origin - // Replace 40 char hash string with "HASH" + // Replace 40-character hexadecimal hash in URLs with "HASH" + // Example: https://example.com/1234567890abcdef1234567890abcdef12345678/path + // becomes: https://example.com/HASH/path .replace(hashUrlRegex(), '$1HASH$3') - // Replace ipv4 address using . separator with "IPV4ADDRESS" + // Replace IPv4 addresses (e.g., 192.168.1.1) with "IPV4ADDRESS" .replace(ipRegex(), 'IPV4ADDRESS') - // Replace ipv4 address using - separator with "IPV4ADDRESS" + // Replace IPv4 addresses with dash separators (e.g., 192-168-1-1) with "IPV4ADDRESS" .replace(new RegExp(ipRegex().toString().replace('\\.', '-')), '/IPV4ADDRESS') - // Report WAN port + // Replace WAN port number with "WANPORT" for consistency .replace(`:${getters.config().remote.wanport || 443}`, ':WANPORT') ) .filter(Boolean); };
270-281: Improve error handling in cloud data fetching.The current implementation mixes async/await with .then/.catch. Consider using try/catch for consistency and better error handling.
- const cloud = await getCloudData(client) - .then((data) => { - this.logger.debug('Cloud Data', data); - return data; - }) - .catch((error) => { - this.logger.debug( - 'Failed fetching cloud from local graphql with "%s"', - error instanceof Error ? error.message : 'Unknown Error' - ); - return null; - }); + let cloud: CloudQueryResult | null = null; + try { + cloud = await getCloudData(client); + this.logger.debug('Cloud Data', cloud); + } catch (error) { + this.logger.debug( + 'Failed fetching cloud from local graphql with "%s"', + error instanceof Error ? error.message : 'Unknown Error' + ); + }
337-356: Extract report template to a separate function.The report template string is complex and mixes formatting with business logic. Consider extracting it to a separate function for better maintainability.
+ private generateReportTemplate(reportObject: ReportObject, v: number): string { + return ` +<-----UNRAID-API-REPORT-----> +SERVER_NAME: ${reportObject.os.serverName} +ENVIRONMENT: ${reportObject.api.environment} +UNRAID_VERSION: ${reportObject.os.version} +UNRAID_API_VERSION: ${reportObject.api.version} +UNRAID_API_STATUS: ${reportObject.api.status} +API_KEY: ${reportObject.apiKey} +MY_SERVERS: ${reportObject.myServers.status}${ + reportObject.myServers.myServersUsername + ? `\nMY_SERVERS_USERNAME: ${reportObject.myServers.myServersUsername}` + : '' + } +CLOUD: ${getReadableCloudDetails(reportObject, v)} +MINI-GRAPH: ${getReadableMinigraphDetails(reportObject)}${getReadableServerDetails( + reportObject, + v + )}${getReadableAllowedOrigins(reportObject)} +</----UNRAID-API-REPORT-----> +`; + } } else { - const report = `...`; // Current template + const report = this.generateReportTemplate(reportObject, v);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/cli/report.command.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/cli/report.command.ts
[error] 239-239: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/cli/report.command.ts (1)
239-239: 🛠️ Refactor suggestionReplace
voidwithundefinedin union type.Using
voidin a union type can be confusing. Replace it withundefinedfor better type clarity.- async report(options?: ReportOptions): Promise<string | ReportObject | void> { + async report(options?: ReportOptions): Promise<string | ReportObject | undefined> {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 239-239: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/src/unraid-api/cli/report.command.ts (5)
25-53: Add JSDoc comments to document the ReportObject type.The
ReportObjecttype would benefit from JSDoc comments explaining the purpose and possible values of each field, especially for fields likestatusthat have specific string literals.+/** + * Represents a comprehensive report of the Unraid API system status. + */ type ReportObject = { + /** Operating system information */ os: { serverName: string; version: string; }; + /** API status and version information */ api: { version: string; status: 'running' | 'stopped'; environment: string; nodeVersion: string; }; + /** API key validation status */ apiKey: 'valid' | 'invalid' | string; servers?: ServersPayload | null; + /** My Servers authentication status */ myServers: { status: 'authenticated' | 'signed out'; myServersUsername?: string; }; // ... rest of the type
56-61: Standardize async/await usage for consistency.The function uses
.then()while other functions use async/await. For consistency, consider using async/await here as well.export const getCloudData = async ( client: ApolloClient<NormalizedCacheObject> ): Promise<CloudQueryResult | null> => { - const cloud = await client.query({ query: getCloudDocument }); - return cloud.data.cloud ?? null; + try { + const cloud = await client.query({ query: getCloudDocument }); + return cloud.data.cloud ?? null; + } catch (error) { + return null; + } };
104-121: Consider using a more maintainable approach for URL anonymization.The current implementation uses multiple regex replacements which could be hard to maintain. Consider using a URL parser and replacing components systematically.
+import { URL } from 'url'; + export const anonymiseOrigins = (origins?: string[]): string[] => { const originsWithoutSocks = origins?.filter((url) => !url.endsWith('.sock')) ?? []; return originsWithoutSocks - .map((origin) => - origin - // Replace 40 char hash string with "HASH" - .replace(hashUrlRegex(), '$1HASH$3') - // Replace ipv4 address using . separator with "IPV4ADDRESS" - .replace(ipRegex(), 'IPV4ADDRESS') - // Replace ipv4 address using - separator with "IPV4ADDRESS" - .replace(new RegExp(ipRegex().toString().replace('\\.', '-')), '/IPV4ADDRESS') - // Report WAN port - .replace(`:${getters.config().remote.wanport || 443}`, ':WANPORT') - ) + .map((origin) => { + try { + const url = new URL(origin); + // Replace IP addresses in hostname + url.hostname = url.hostname.replace(ipRegex(), 'IPV4ADDRESS'); + // Replace hash in pathname + url.pathname = url.pathname.replace(hashUrlRegex(), '$1HASH$3'); + // Replace WAN port + if (url.port === String(getters.config().remote.wanport || 443)) { + url.port = 'WANPORT'; + } + return url.toString(); + } catch { + return origin; + } + }) .filter(Boolean); };
204-208: Improve error handling and default options structure.The report method's error handling could be more robust, and the default options could be better structured:
- Consider using a dedicated error type for better error handling
- Move default options to a static property or constant
+const DEFAULT_REPORT_OPTIONS: ReportOptions = { + raw: false, + json: false, + verbose: 0, +} as const; + +class ReportError extends Error { + constructor(message: string, public readonly cause?: unknown) { + super(message); + this.name = 'ReportError'; + } +} + @Command({ name: 'report' }) export class ReportCommand extends CommandRunner { - private defaultOptions: ReportOptions = { - raw: false, - json: false, - verbose: 0, - }; + private static readonly defaultOptions = DEFAULT_REPORT_OPTIONS; async report( - options: ReportOptions = this.defaultOptions + options: ReportOptions = ReportCommand.defaultOptions ): Promise<string | ReportObject | undefined> { try { // ... existing code ... } catch (error: unknown) { - if (error instanceof Error) { - this.logger.debug(error); - this.logger.error(`\nFailed generating report with "${error.message}"\n`); - return; - } else { - this.logger.error('Failed generating report'); - return; - } + const reportError = error instanceof Error + ? new ReportError(error.message, error) + : new ReportError('Failed generating report', error); + this.logger.debug(reportError); + this.logger.error(`\nFailed generating report with "${reportError.message}"\n`); + return undefined; } }Also applies to: 245-377
275-286: Improve error handling in cloud data fetching.The error handling in the cloud data fetching could be more robust. Consider using a try-catch block instead of promise chaining for consistency with the rest of the codebase.
- const cloud = await getCloudData(client) - .then((data) => { - this.logger.debug('Cloud Data', data); - return data; - }) - .catch((error) => { - this.logger.debug( - 'Failed fetching cloud from local graphql with "%s"', - error instanceof Error ? error.message : 'Unknown Error' - ); - return null; - }); + let cloud: CloudQueryResult | null = null; + try { + cloud = await getCloudData(client); + this.logger.debug('Cloud Data', cloud); + } catch (error) { + this.logger.debug( + 'Failed fetching cloud from local graphql with "%s"', + error instanceof Error ? error.message : 'Unknown Error' + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/cli/report.command.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/cli/report.command.ts
[error] 245-245: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/cli/report.command.ts (1)
245-245: 🛠️ Refactor suggestionReplace
voidwithundefinedin union type.Using
voidin a union type can be confusing. Replace it withundefinedfor better type clarity.- async report(options: ReportOptions = this.defaultOptions): Promise<string | ReportObject | void> { + async report(options: ReportOptions = this.defaultOptions): Promise<string | ReportObject | undefined> {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 245-245: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/src/cli.ts (2)
15-15: Consider enabling the LogService for better debugging.The commented code suggests a debugging option. Consider making this configurable via an environment variable or command-line flag.
- logger: false, // new LogService(), - enable this to see nest initialization issues + logger: process.env.DEBUG === 'true' ? new LogService() : false,
22-22: Remove redundant process.exit(0).The explicit
process.exit(0)is unnecessary as the process would naturally exit after successful completion.- process.exit(0);api/dev/states/myservers.cfg (1)
20-20: Review security implications of exposed configuration.Several security considerations to address:
- Consider separating development and production configurations
- Review if internal URLs in
allowedOriginsshould be exposed in example config- Ensure all sensitive fields are consistently masked with placeholders
Consider implementing:
- Environment-specific configuration files
- A validation mechanism for allowed origins
- Documentation for security-sensitive fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json(1 hunks)api/dev/states/myservers.cfg(1 hunks)api/src/cli.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/cli.ts (3)
4-9: LGTM! Well-organized imports.The imports are properly organized and include all necessary dependencies for the CLI functionality.
13-21: Wrap top-levelawaitin an async function for compatibility.Using
awaitat the top level requires Node.js 14+ and ECMAScript modules. To ensure compatibility across different environments, wrap the code in an immediately invoked async function.+const main = async () => { try { const shellToUse = execSync('which unraid-api').toString().trim(); await CommandFactory.run(CliModule, { cliName: 'unraid-api', logger: false, completion: { fig: true, cmd: 'unraid-api', nativeShell: { executablePath: shellToUse }, }, }); } catch (error) { cliLogger.error('ERROR:', error); internalLogger.error({ message: 'Failed to start unraid-api', error, }); process.exit(1); } }; + +main();
12-12:⚠️ Potential issueAdd error handling for the
execSynccommand.The code assumes
unraid-apiis available in PATH. Add validation and provide a helpful error message if the command is not found.- const shellToUse = execSync('which unraid-api').toString().trim(); + const shellToUse = execSync('which unraid-api').toString().trim(); + if (!shellToUse) { + throw new Error('unraid-api command not found in PATH. Please ensure it is installed correctly.'); + }Likely invalid or redundant comment.
api/dev/states/myservers.cfg (3)
19-20: Standardize placeholder values for sensitive fields.For consistency with other configuration examples, consider using a descriptive placeholder for the
ssoSubIdsfield.-ssoSubIds="" +ssoSubIds="<sso_sub_ids_placeholder>"
23-23: Document the relationship between UPnP configuration fields.Please clarify the relationship between
upnpStatusandupnpEnabledfields. Consider adding comments to explain:
- The purpose of each field
- How they interact with each other
- Valid values for the status field
16-16: Ensure consistent handling of sensitive token fields.The new
idtokenfield should follow the same security practices as other sensitive fields. Consider using a placeholder value or documentation to indicate the expected format.
| process.exit(0); | ||
| } catch (error) { | ||
| console.log(error); | ||
| cliLogger.error('ERROR:', error); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging.
The current error handling could expose sensitive information and lacks error-specific messaging.
- cliLogger.error('ERROR:', error);
+ const errorMessage = error.code === 'ENOENT'
+ ? 'unraid-api command not found. Please ensure it is installed correctly.'
+ : 'Failed to execute unraid-api command.';
+ cliLogger.error('ERROR:', errorMessage);
internalLogger.error({
message: 'Failed to start unraid-api',
- error,
+ error: {
+ code: error.code,
+ message: error.message,
+ },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cliLogger.error('ERROR:', error); | |
| const errorMessage = error.code === 'ENOENT' | |
| ? 'unraid-api command not found. Please ensure it is installed correctly.' | |
| : 'Failed to execute unraid-api command.'; | |
| cliLogger.error('ERROR:', errorMessage); | |
| internalLogger.error({ | |
| message: 'Failed to start unraid-api', | |
| error: { | |
| code: error.code, | |
| message: error.message, | |
| }, | |
| }); |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/cli.ts (1)
17-17: Convert code comment to proper documentation.The commented code about LogService initialization would be more helpful as a proper comment.
- logger: false, // new LogService(), - enable this to see nest initialization issues + // Set to new LogService() to debug nest initialization issues + logger: false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/cli.ts(1 hunks)api/src/unraid-api/cli/sso/add-sso-user.command.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/cli/sso/add-sso-user.command.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/cli.ts (3)
5-9: LGTM! Clean and well-organized imports.The imports are appropriate for the CLI functionality being implemented.
26-26: Improve error handling and logging.The current error handling could expose sensitive information and lacks error-specific messaging.
The previous review comment about improving error handling is still valid and should be addressed.
19-21: Verify completion script configuration.The completion script command ('completion-script') differs from the CLI name ('unraid-api'), which might cause issues with shell completion.
| const shellToUse = await execa('which unraid-api') | ||
| .then((res) => res.toString().trim()) | ||
| .catch((_) => '/usr/local/bin/unraid-api'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance shell path determination robustness.
While the code handles the case where which fails, it doesn't validate if the fallback path exists and is executable.
Consider this improved implementation:
- const shellToUse = await execa('which unraid-api')
- .then((res) => res.toString().trim())
- .catch((_) => '/usr/local/bin/unraid-api');
+ const shellToUse = await execa('which unraid-api')
+ .then((res) => res.toString().trim())
+ .catch(async (_) => {
+ const fallbackPath = '/usr/local/bin/unraid-api';
+ try {
+ await execa('test', ['-x', fallbackPath]);
+ return fallbackPath;
+ } catch {
+ throw new Error(`unraid-api executable not found in PATH and fallback path ${fallbackPath} is not executable`);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shellToUse = await execa('which unraid-api') | |
| .then((res) => res.toString().trim()) | |
| .catch((_) => '/usr/local/bin/unraid-api'); | |
| const shellToUse = await execa('which unraid-api') | |
| .then((res) => res.toString().trim()) | |
| .catch(async (_) => { | |
| const fallbackPath = '/usr/local/bin/unraid-api'; | |
| try { | |
| await execa('test', ['-x', fallbackPath]); | |
| return fallbackPath; | |
| } catch { | |
| throw new Error(`unraid-api executable not found in PATH and fallback path ${fallbackPath} is not executable`); | |
| } | |
| }); |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
Release Notes: Unraid API v0.1.0
New Features
Improvements
Changes
Bug Fixes