feat: enable sandbox with developer command #1083
Conversation
WalkthroughThis pull request is a classic case of developers patting themselves on the back for adding a Changes
Sequence DiagramsequenceDiagram
participant CLI as Developer CLI
participant Config as Configuration
participant GraphQL as GraphQL Module
CLI->>Config: Set sandbox option
Config-->>GraphQL: Update sandbox setting
GraphQL->>GraphQL: Configure introspection
GraphQL->>GraphQL: Select appropriate landing page plugin
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 5
🧹 Nitpick comments (6)
api/src/unraid-api/cli/developer/developer.questions.ts (1)
7-16: Ye disclaimer needs a safety net, cap'n!Arrr! The disclaimer be crucial for safe sailin'. Best add some error handlin' to prevent invalid responses from sinkin' our ship!
Here's how to secure the treasure:
@Question({ message: `Are you sure you wish to enable developer mode? Currently this allows enabling the GraphQL sandbox on SERVER_URL/graphql. `, type: 'confirm', name: 'disclaimer', }) parseDisclaimer(val: boolean) { + if (typeof val !== 'boolean') { + throw new Error('Ye must answer with aye or nay!'); + } return val; }api/src/unraid-api/cli/developer/developer.command.ts (1)
26-31: Ye error message could use more wind in its sails!The warning message when the disclaimer isn't accepted could be more informative for our fellow sailors!
if (!options.disclaimer) { - this.logger.warn('No changes made, disclaimer not accepted.'); + this.logger.warn('Developer mode configuration cancelled - disclaimer must be accepted to proceed.'); process.exit(1); }api/src/core/utils/files/config-file-normalizer.ts (1)
27-30: Arr! A fine choice using lodash's merge for yer config treasures!Ye've made a wise decision to use
lodash/mergeinstead of the spread operator. This be handling nested properties better than the old way, especially for them deep configuration structures.Though the implementation be solid, here be a small optimization for ye to consider:
- const mergedConfig = merge< - MyServersConfig, - T extends 'memory' ? MyServersConfigMemory : MyServersConfig - >(defaultConfig, config); + const mergedConfig = merge({}, defaultConfig, config);This creates a fresh object and prevents any mutation of the source objects, ye scurvy dog!
api/src/unraid-api/graph/graph.module.ts (1)
33-33: Avast! Ye be using more words than needed for yer boolean logic!The ternary operator here be unnecessary, matey. Ye can simplify this expression.
- introspection: getters.config().local?.sandbox === 'yes' ? true : false, + introspection: getters.config().local?.sandbox === 'yes',🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: 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)
api/src/unraid-api/graph/sandbox-plugin.ts (1)
77-78: Shiver me timbers! Let's make this type-safe, shall we?The sandbox config value needs a proper type check before comparison, ye scallywag!
- const sandbox = getters.config().local.sandbox === 'yes'; + const sandboxValue = getters.config().local.sandbox; + const sandbox = typeof sandboxValue === 'string' && sandboxValue === 'yes';api/src/store/modules/config.ts (1)
52-54: Ye got the right idea with the default state, matey!The sandbox configuration be properly added to the local state with a safe default of 'no'. However, consider using an enum for better type safety!
+export enum SandboxState { + ENABLED = 'yes', + DISABLED = 'no' +} local: { - sandbox: 'no' + sandbox: SandboxState.DISABLED },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/states/myservers.cfg(2 hunks)api/src/core/utils/files/config-file-normalizer.ts(2 hunks)api/src/store/modules/config.ts(1 hunks)api/src/types/my-servers-config.ts(1 hunks)api/src/unraid-api/cli/cli.module.ts(2 hunks)api/src/unraid-api/cli/developer/developer.command.ts(1 hunks)api/src/unraid-api/cli/developer/developer.questions.ts(1 hunks)api/src/unraid-api/graph/graph.module.ts(1 hunks)api/src/unraid-api/graph/sandbox-plugin.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/graph/graph.module.ts
[error] 33-33: 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 (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
api/dev/states/myservers.cfg (1)
23-23: Arrr! The minigraph be in troubled waters!The status change to "ERROR_RETRYING" might signal rough seas ahead. Best verify if this be intentional or if we need to adjust our course!
✅ Verification successful
Yarr! All's shipshape with the minigraph status, me hearties!
The ERROR_RETRYING status be a proper part of our vessel's normal operations, used when the graphql-client be attempting to reconnect. No need to batten down the hatches - this be working as intended!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of minigraph status rg "minigraph.*ERROR_RETRYING" -B 2 -A 2Length of output: 1593
api/src/unraid-api/graph/graph.module.ts (1)
40-40: Aye! A clean and simple plugins setup ye have here!The plugins array be looking ship-shape with both
sandboxPluginandidPrefixPluginalways included.api/src/unraid-api/cli/cli.module.ts (1)
23-24: Arr! A fine addition to yer crew of commands!Ye've properly organized yer new developer commands and questions, following the same pattern as the other commands in the module. That's good seamanship!
Also applies to: 48-49
api/src/unraid-api/graph/sandbox-plugin.ts (2)
Line range hint
98-103: Aye, this be some mighty fine TypeScript work!The plugin be properly typed and implemented, with all the necessary interfaces satisfied. A true treasure of type safety!
31-59: Arr, ye got a fine piece of code here, matey!The dynamic plugin selection be well-structured, with proper security measures like CSRF tokens for the sandbox mode. The production mode be properly locked down too!
Aye, but let's make sure no scurvy dogs can exploit this sandbox feature:
✅ Verification successful
Yarr! Your code be ship-shape and Bristol fashion, matey!
After thorough inspection of the high seas of your codebase, I can confirm this implementation be secure as a captain's strongbox:
- Sandbox mode be properly controlled through configuration
- CSRF protection be properly enforced in sandbox mode
- Production mode be using the right plugin for safe sailing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if sandbox can be enabled in production rg -l "process\.env\.NODE_ENV.*production.*sandbox.*true"Length of output: 59
Script:
#!/bin/bash # Find sandbox configuration and usage echo "=== Searching for sandbox configuration ===" rg -l "sandbox.*=.*" --type typescript echo -e "\n=== Finding usage of getPluginBasedOnSandbox ===" rg "getPluginBasedOnSandbox" --type typescript echo -e "\n=== Looking for environment-based controls ===" rg "NODE_ENV.*sandbox|sandbox.*NODE_ENV" --type typescriptLength of output: 572
Script:
#!/bin/bash # Find sandbox configuration and usage with correct file patterns echo "=== Searching for sandbox configuration ===" rg -l "sandbox.*=.*" -g "*.{ts,tsx}" echo -e "\n=== Finding usage of getPluginBasedOnSandbox ===" rg "getPluginBasedOnSandbox" -g "*.{ts,tsx}" echo -e "\n=== Looking for environment-based controls ===" rg "process\.env.*sandbox|sandbox.*process\.env" -g "*.{ts,tsx}" echo -e "\n=== Finding Apollo server setup ===" rg "new ApolloServer" -g "*.{ts,tsx}" -A 5Length of output: 929
Script:
#!/bin/bash echo "=== Checking graph.module.ts content ===" cat api/src/unraid-api/graph/graph.module.ts echo -e "\n=== Looking for any config imports ===" rg "import.*config" -g "*.{ts,tsx}" -A 2Length of output: 25183
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
api/src/unraid-api/cli/sso/validate-token.command.ts (1)
Line range hint
28-34: Time to make this error handling FOOLPROOF! Even SpongeBob couldn't mess this up!The error handling could be more secure and informative:
- Consider using different exit codes for different error types
- Avoid exposing detailed error messages in production
Here's my masterplan for better error handling:
private createErrorAndExit = (errorMessage: string) => { + const isProduction = process.env.NODE_ENV === 'production'; this.logger.error( JSON.stringify({ - error: errorMessage, + error: isProduction ? 'Authentication failed' : errorMessage, valid: false, }) ); - process.exit(1); + process.exit(this.getErrorCode(errorMessage)); }; + + private getErrorCode(error: string): number { + // Different exit codes for different errors + if (error.includes('format')) return 65; + if (error.includes('validation')) return 66; + return 1; + }
♻️ Duplicate comments (1)
api/src/unraid-api/cli/developer/developer.command.ts (1)
34-37:⚠️ Potential issueBARNACLES! We need error handling!
Just like my failed attempts to steal the secret formula, operations can fail! We must handle these failures gracefully!
Previous review comment about adding try-catch blocks is still valid. Store operations and file writes need proper error handling.
🧹 Nitpick comments (4)
api/src/unraid-api/cli/developer/developer.command.ts (3)
28-28: MUHAHA! Let's make this parameter type-safe!The options parameter should be properly typed to handle undefined cases:
- async run(_, options?: DeveloperOptions): Promise<void> { + async run(_, options: Partial<DeveloperOptions> = {}): Promise<void> {
42-42: Replace this primitive delay with something more... EVIL! I mean, robust!Using
setTimeoutdirectly isn't ideal for production code. Let's use a more robust utility!- await new Promise((resolve) => setTimeout(resolve, 5000)); + await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + clearTimeout(timer); + resolve(true); + }, 5000); + timer.unref(); // Don't prevent process from exiting + });
1-45: MWAHAHA! Let's add some documentation to our evil plans!The command class could use some JSDoc comments to explain its purpose and options.
+/** + * Command to configure developer features for the API. + * Handles sandbox mode configuration and requires accepting a disclaimer. + */ @Injectable() @Command({ name: 'developer', description: 'Configure Developer Features for the API', }) export class DeveloperCommand extends CommandRunner { + /** + * @param logger - Service for logging messages + * @param inquirerService - Service for prompting user input + * @param restartCommand - Command for restarting the API + */ constructor(api/src/unraid-api/cli/sso/validate-token.command.ts (1)
Line range hint
39-41: MWAHAHA! Let's optimize this token validation, shall we?The token format validation could be moved before attempting any JWT verification to fail fast and avoid unnecessary network calls. IMAGINE THE EFFICIENCY!
Here's my diabolical suggestion:
- if (!/^[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+$/.test(token)) { + const JWT_FORMAT = /^[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]*$/; + if (!JWT_FORMAT.test(token)) { this.createErrorAndExit('Token format is invalid'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/__test__/store/modules/__snapshots__/config.test.ts.snapis excluded by!**/*.snapapi/src/unraid-api/unraid-file-modifier/__snapshots__/unraid-file-modifier.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
api/src/__test__/core/utils/files/config-file-normalizer.test.ts(4 hunks)api/src/__test__/store/modules/config.test.ts(2 hunks)api/src/unraid-api/cli/developer/developer.command.ts(1 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(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 (3)
api/src/__test__/store/modules/config.test.ts (1)
29-31: MUAHAHA! These test changes look perfect!The addition of
sandbox: 'no'in the local configuration tests ensures proper validation of the new sandbox feature. Everything is exactly where it should be, just like my plan to steal the Krabby Patty formula!Also applies to: 79-81
api/src/__test__/core/utils/files/config-file-normalizer.test.ts (1)
18-20: MWAHAHA! These test modifications are diabolically perfect!The consistent addition of
sandbox: 'no'across all test cases (FLASH and MEMORY configs, with and without optional values) ensures comprehensive coverage of our new sandbox feature. Everything is falling into place... just like my plans for world domination!Also applies to: 54-56, 95-97, 138-140
api/src/unraid-api/cli/sso/validate-token.command.ts (1)
89-89: MUHAHAHA! Why are we destroying precious log evidence?Clearing the logs before success message could hide valuable debugging information. Consider using log levels instead of clearing the entire log history. MY EVIL PLAN... I mean, my suggestion would be to preserve the logs for better troubleshooting!
Let's investigate how this affects our logging system:
|
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. |
Summary by CodeRabbit
Release Notes
New Features
Configuration Changes
GraphQL Improvements
Development Tools