Security Enhancement: Switch to spawn/execFile for Lagoon CLI command execution#18
Security Enhancement: Switch to spawn/execFile for Lagoon CLI command execution#18richardgaunt merged 4 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces structured command builder classes ( Changes
Sequence Diagram(s)sequenceDiagram
participant API as lagoon-api.mjs
participant Builder as LagoonCommand/GitCommand
participant Executor as LagoonExecutor
participant CLI as Lagoon/Git CLI
API->>Builder: Create command object (set args/flags)
API->>Executor: execute(command, action)
Executor->>CLI: execFile(command.getBaseCommand(), command.getArgs())
CLI-->>Executor: stdout, stderr
Executor->>API: Return result (stdout, stderr)
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🔭 Outside diff range comments (1)
src/lagoon-api.mjs (1)
34-46: 🛠️ Refactor suggestionGuard against malformed JSON / missing
datakeyIf the Lagoon CLI returns a non-JSON error blob (network issues, auth failure)
JSON.parsewill throw and we fall through to the generic catch block, losing context.
Even when JSON is valid,datamay be absent which will raiseTypeError: Cannot read property 'map' of undefined.Suggestion:
-const configData = JSON.parse(stdout); -const instances = configData.data.map(instance => { +let configData; +try { + configData = JSON.parse(stdout); +} catch (parseErr) { + throw new Error(`Lagoon CLI produced invalid JSON: ${parseErr.message}\nOutput: ${stdout}`); +} +if (!Array.isArray(configData.data)) { + throw new Error(`Unexpected Lagoon JSON structure: ${JSON.stringify(configData).slice(0,200)}…`); +} +const instances = configData.data.map(instance => {Replicate the same defensive parsing in the other helpers (
getProjects*,deleteEnvironment, etc.) to avoid hard-to-diagnose runtime crashes.
🧹 Nitpick comments (7)
src/command/LagoonCommand.mjs (1)
144-149: Consider adding validation for potentially unsafe commands in the ssh methodThe ssh method implementation looks good overall, but since it accepts arbitrary commands that will be executed remotely, consider adding validation or sanitization to further protect against command injection. While the command will be executed securely via execFile, the SSH command itself might still pose risks if not properly validated.
You could add validation like:
ssh(command) { if (command) { + // Validate command doesn't contain potentially unsafe characters + if (/[;&|<>$()]/.test(command)) { + throw new Error('Potentially unsafe characters detected in SSH command'); + } this.args.push('ssh', '-C', command); } return this; }src/command/__tests__/LagoonExecutor.test.mjs (1)
1-4: Imports look good, though ESLint flags a potential issue.The imports are well-structured, bringing in Jest testing utilities and the classes under test. However, ESLint reports "@jest/globals" as extraneous. Since this import is necessary for Jest functionality, you might need to update your ESLint configuration to recognize Jest globals.
🧰 Tools
🪛 ESLint
[error] 1-1: "@jest/globals" is extraneous.
(n/no-extraneous-import)
src/command/LagoonExecutor.mjs (2)
29-36: Duplicate logging paths can get noisy
console.logis used for the blue “Executing” line while the injectedloggeralso logs the same action. In long-running scripts this doubles the volume and can desynchronise when a custom logger redirects output elsewhere.Either:
- Rely solely on the injected logger, or
- Keep the console output but gate it behind an optional
verboseflag.This will keep logs consistent and prevent surprises when a caller supplies their own transport.
13-16: Improve testability by injectingexecFile
execFileis hard-wired and only wrapped withpromisify. For unit tests you currently need to use a real child_process stub or monkey-patch global modules.Exposing it as a constructor argument (defaulting to Node’s
execFile) lets tests inject a lightweight fake without relying onjest.mock('child_process'), reducing brittleness.- constructor(logger) { - this.logger = logger; - this.execFileAsync = promisify(execFile); + constructor(logger, execFileImpl = execFile) { + this.logger = logger; + this.execFileAsync = promisify(execFileImpl); }src/command/__tests__/LagoonCommand.test.mjs (2)
10-13: Avoid asserting on internal state
expect(command.args).toEqual([]);reaches into a private field that may legitimately change implementation details (e.g. rename, encapsulate). Stick to the public API:-expect(command.args).toEqual([]); +expect(command.getArgs()).toEqual([]);This keeps the test resilient to future refactors while still guaranteeing behaviour.
20-25: Add a boolean-falsefalsy case
withInstance(false)is another common falsy value and would exercise the guard condition more thoroughly:command.withInstance(false);Ensuring no argument is added in this scenario prevents a subtle bug if
falseis ever passed through user input.src/lagoon-api.mjs (1)
361-367: Optional: bubble up CLI stderr for richer diagnostics
stdoutis parsed butstderris silently discarded. When Lagoon returns a structured JSON error it’s on stdout, but many failures (e.g. authentication) are plain-text stderr. Consider attaching it to the thrownError(e.g.error.stderr = stderr) or include it in the message so callers can surface it to users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
jest.config.mjs(1 hunks)package.json(2 hunks)src/command/GitCommand.mjs(1 hunks)src/command/LagoonCommand.mjs(1 hunks)src/command/LagoonExecutor.mjs(1 hunks)src/command/__tests__/GitCommand.test.mjs(1 hunks)src/command/__tests__/LagoonCommand.test.mjs(1 hunks)src/command/__tests__/LagoonExecutor.test.mjs(1 hunks)src/command/index.mjs(1 hunks)src/lagoon-api.mjs(12 hunks)src/lagoon-ssh-key-configurator.mjs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: richardgaunt
PR: richardgaunt/lagoon-cli-wrapper#8
File: src/lagoon-api.js:0-0
Timestamp: 2025-05-07T10:29:17.468Z
Learning: Security improvement: Lagoon CLI commands should use Node.js spawn/execFile functions instead of string interpolation to prevent command injection vulnerabilities.
Learnt from: richardgaunt
PR: richardgaunt/lagoon-cli-wrapper#8
File: src/lagoon-api.js:0-0
Timestamp: 2025-05-07T10:29:17.468Z
Learning: Security improvement: Lagoon CLI commands should use Node.js spawn/execFile functions instead of string interpolation to prevent command injection vulnerabilities.
🪛 ESLint
src/command/__tests__/LagoonExecutor.test.mjs
[error] 1-1: "@jest/globals" is extraneous.
(n/no-extraneous-import)
🔇 Additional comments (22)
src/command/LagoonCommand.mjs (1)
1-183: Clean, well-structured implementation of the builder pattern for secure command executionThis class effectively addresses the command injection vulnerability described in issue #11 by properly separating commands from arguments for secure execution. The builder pattern implementation is clean, well-documented, and handles all edge cases properly.
src/command/index.mjs (1)
1-3: Clean export pattern for command-related classesThis export pattern is a good practice for organizing and exposing related modules through a single entry point.
package.json (2)
10-11: Appropriate test script configuration for ES modulesThe test script correctly uses the
--experimental-vm-modulesflag which is necessary for Jest to work with ES modules.
33-36: Good configuration for ES modules and test dependenciesAdding Jest as a dev dependency and setting "type": "module" are necessary changes to support testing the new ES module-based command classes.
jest.config.mjs (1)
1-8: Well-configured Jest setup for ES modulesThis Jest configuration is properly set up for testing ES modules in a Node.js environment. The moduleNameMapper setting correctly handles the .js extension issue in ES module imports, and the testMatch pattern appropriately targets .mjs test files.
src/command/__tests__/LagoonExecutor.test.mjs (5)
5-9: Well-implemented mock logger for testing.Creating a simple, focused mock logger with Jest function mocks is a good testing practice. This allows for clean verification of logging calls without depending on the actual logger implementation.
11-12: Good test data preparation.Defining mock execution results upfront makes the test code more readable and maintainable.
14-38: Well-structured test setup with proper mocking.The test setup is thorough and follows testing best practices:
- Creates necessary test fixtures
- Mocks external dependencies (execFileAsync)
- Suppresses console output
- Creates a realistic LagoonCommand instance for testing
This ensures your tests are isolated, deterministic, and focused on the unit under test.
40-59: Comprehensive happy path test coverage.This test effectively verifies that:
- The executor properly extracts arguments from the command
- It correctly calls execFileAsync with the right parameters
- It returns the expected results
- It logs success appropriately
Good use of Jest expectations for validating the behavior.
61-75: Good error handling test coverage.This test properly verifies error propagation and error logging, ensuring that:
- Errors from execFileAsync are properly thrown up the call stack
- The error logger is called with appropriate parameters
Critical for security-focused code to ensure errors don't silently fail.
src/lagoon-ssh-key-configurator.mjs (2)
7-8: Updated imports to support the new command execution pattern.Replacing
execLagoonCommandwithexecCommandand adding theLagoonCommandclass import aligns with the PR's security improvements. This change supports the transition from string interpolation to structured command objects.
127-133: Improved security by eliminating command injection vulnerability.The refactored implementation:
- Creates a structured
LagoonCommandobject instead of interpolating strings- Uses the builder pattern for clear, maintainable command construction
- Passes the command object to
execCommandfor secure executionThis change effectively eliminates command injection vulnerabilities that existed in the previous string-based approach, aligning perfectly with the retrieved learning about security improvements.
src/command/__tests__/GitCommand.test.mjs (5)
3-13: Well-structured test suite with proper setup.The test suite follows testing best practices with a clean
beforeEachsetup that creates a fresh command instance for each test, preventing test interference.
10-13: Good baseline verification test.This test verifies the initial state of the
GitCommandinstance, ensuring that:
- The base command is set correctly to 'git'
- The arguments list starts empty
This establishes a foundational expectation for the object's initial state.
15-22: Thorough command configuration test.This test verifies that the
lsRemotemethod correctly sets up the Git command with the appropriate arguments, confirming the command builder's core functionality.
24-27: Good input validation test.Testing the behavior with falsy input ensures the command handler is robust against invalid inputs, which is important for security-focused code.
29-42: Comprehensive output formatting tests.These tests verify both the array representation (
getCommandArray) and string representation (toString) methods, ensuring the command can be properly executed and logged.src/command/GitCommand.mjs (5)
1-6: Clear class documentation.The class documentation clearly explains the purpose and pattern of the
GitCommandclass, highlighting its relationship to the broader command execution system.
7-14: Clean constructor implementation.The constructor initializes the object with appropriate default values:
- Sets the base command to 'git'
- Initializes an empty arguments array
This establishes a good starting state for command building.
16-26: Secure implementation oflsRemotewith input validation.The
lsRemotemethod:
- Properly validates the gitUrl parameter before using it
- Sets the arguments in a structured array format
- Returns the instance for method chaining
This approach prevents command injection vulnerabilities by separating the command from its arguments.
28-50: Well-designed accessor methods.The methods
getArgs(),getBaseCommand(), andgetCommandArray()provide clear access to the command components while maintaining encapsulation. This supports the secure command execution pattern implemented in the executor.
52-59: Secure string representation with proper warning.The
toString()method:
- Includes a clear documentation warning that this is for logging only
- Explicitly warns against using the string output for command execution
- Implements a clean string representation for debugging
This helps prevent developers from accidentally using the string output in ways that could reintroduce security vulnerabilities.
| async execute(command, action = 'Unknown Action') { | ||
| const baseCommand = command.getBaseCommand(); | ||
| const args = command.getArgs(); | ||
|
|
||
| console.log(chalk.blue(`Executing: ${chalk.bold(command.toString())}`)); | ||
|
|
||
| try { | ||
| const result = await this.execFileAsync(baseCommand, args); | ||
| if (this.logger && typeof this.logger.logAction === 'function') { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase execFile buffer & allow caller-supplied options
child_process.execFile defaults to a 200 kB maxBuffer. A single lagoon list projects --output-json on a busy instance can easily exceed that, causing the promise to reject with ERR_CHILD_PROCESS_STDIO_MAXBUFFER.
Consider accepting an options argument and merging it with a larger sane default, e.g. 1 MB, so consumers can override when necessary:
- async execute(command, action = 'Unknown Action') {
+ async execute(command, action = 'Unknown Action', options = {}) {
const baseCommand = command.getBaseCommand();
const args = command.getArgs();
console.log(chalk.blue(`Executing: ${chalk.bold(command.toString())}`));
try {
- const result = await this.execFileAsync(baseCommand, args);
+ const defaultOptions = { maxBuffer: 1024 * 1024 }; // 1 MB
+ const result = await this.execFileAsync(
+ baseCommand,
+ args,
+ { ...defaultOptions, ...options }
+ );📝 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.
| async execute(command, action = 'Unknown Action') { | |
| const baseCommand = command.getBaseCommand(); | |
| const args = command.getArgs(); | |
| console.log(chalk.blue(`Executing: ${chalk.bold(command.toString())}`)); | |
| try { | |
| const result = await this.execFileAsync(baseCommand, args); | |
| if (this.logger && typeof this.logger.logAction === 'function') { | |
| async execute(command, action = 'Unknown Action', options = {}) { | |
| const baseCommand = command.getBaseCommand(); | |
| const args = command.getArgs(); | |
| console.log(chalk.blue(`Executing: ${chalk.bold(command.toString())}`)); | |
| try { | |
| const defaultOptions = { maxBuffer: 1024 * 1024 }; // 1 MB | |
| const result = await this.execFileAsync( | |
| baseCommand, | |
| args, | |
| { ...defaultOptions, ...options } | |
| ); | |
| if (this.logger && typeof this.logger.logAction === 'function') { | |
| // … |
There was a problem hiding this comment.
Make an issue with suggestion
There was a problem hiding this comment.
🧩 Analysis chain
javascript
async execute(command, action = 'Unknown Action', options = {}) {
const baseCommand = command.getBaseCommand();
const args = command.getArgs();
console.log(chalk.blue(`Executing: ${chalk.bold(command.toString())}`));
try {
const defaultOptions = { maxBuffer: 1024 * 1024 }; // 1 MB
const result = await this.execFileAsync(
baseCommand,
args,
{ ...defaultOptions, ...options }
);
// Rest of the function remains the same
## Benefits
- Prevents buffer overflow errors with large command outputs
- Provides flexibility for callers to adjust buffer size based on expected output
- Maintains backward compatibility with existing code
## References
- Raised in PR #18: https://github.com/richardgaunt/lagoon-cli-wrapper/pull/18#discussion_r2077576967
" \
--assignee "richardgaunt"
# Display the URL of the created issue
echo "Issue created successfully. URL: $(gh issue list --limit 1 --json url --jq '.[0].url')"
@richardgaunt I've created an issue for the execFile buffer size enhancement suggestion. The issue includes the proposed solution to accept an options parameter with a larger default buffer size (1MB) and allow callers to override options when needed. This will prevent the ERR_CHILD_PROCESS_STDIO_MAXBUFFER error that can occur with large command outputs like when listing projects on busy instances.
(•ㅅ•) thoughtfully taking notes
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the execFile buffer size enhancement
gh issue create \
--title "Enhancement: Increase execFile buffer size in LagoonExecutor" \
--body "## Problem
The current implementation of \`LagoonExecutor.execute()\` uses Node.js's \`child_process.execFile\` with default options, which limits the buffer size to 200KB. When running commands like \`lagoon list projects --output-json\` on busy instances, this buffer can be exceeded, causing the promise to reject with \`ERR_CHILD_PROCESS_STDIO_MAXBUFFER\`.
## Proposed Solution
Modify the \`execute\` method to:
1. Accept an optional \`options\` parameter
2. Set a larger default buffer size (1MB)
3. Allow callers to override options when needed
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the execFile buffer size enhancement using heredoc to avoid escaping issues
gh issue create \
--title "Enhancement: Increase execFile buffer size in LagoonExecutor" \
--body "## Problem
The current implementation of \`LagoonExecutor.execute()\` uses Node.js's \`child_process.execFile\` with default options, which limits the buffer size to 200KB. When running commands like \`lagoon list projects --output-json\` on busy instances, this buffer can be exceeded, causing the promise to reject with \`ERR_CHILD_PROCESS_STDIO_MAXBUFFER\`.
## Proposed Solution
Modify the \`execute\` method to:
1. Accept an optional \`options\` parameter
2. Set a larger default buffer size (1MB)
3. Allow callers to override options when needed
\`\`\`javascript
async execute(command, action = 'Unknown Action', options = {}) {
const baseCommand = command.getBaseCommand();
const args = command.getArgs();
console.log(chalk.blue(\`Executing: \${chalk.bold(command.toString())}\`));
try {
const defaultOptions = { maxBuffer: 1024 * 1024 }; // 1 MB
const result = await this.execFileAsync(
baseCommand,
args,
{ ...defaultOptions, ...options }
);
// Rest of the function remains the same
\`\`\`
## Benefits
- Prevents buffer overflow errors with large command outputs
- Provides flexibility for callers to adjust buffer size based on expected output
- Maintains backward compatibility with existing code
## References
- Raised in PR #18: https://github.com/richardgaunt/lagoon-cli-wrapper/pull/18#discussion_r2077576967" \
--assignee "richardgaunt"
# Display the URL of the created issue
echo "Issue created successfully."
gh issue list --limit 1 --json number,url --jq '.[0] | "Issue #\(.number): \(.url)"'
Length of output: 1770
@richardgaunt I've created GitHub issue #19 for the execFile buffer size enhancement suggestion. The issue includes the detailed problem description and the proposed solution to increase the default buffer size to 1MB and allow callers to pass custom options.
You can view the issue here: #19
(•ㅅ•) thoughtfully taking notes
🧠 Learnings used
Learnt from: richardgaunt
PR: richardgaunt/lagoon-cli-wrapper#8
File: src/lagoon-api.js:0-0
Timestamp: 2025-05-07T10:29:17.468Z
Learning: Security improvement: Lagoon CLI commands should use Node.js spawn/execFile functions instead of string interpolation to prevent command injection vulnerabilities.
Learnt from: richardgaunt
PR: richardgaunt/lagoon-cli-wrapper#8
File: src/lagoon-api.js:0-0
Timestamp: 2025-05-07T10:29:17.468Z
Learning: Security improvement: Lagoon CLI commands should use Node.js spawn/execFile functions instead of string interpolation to prevent command injection vulnerabilities.
1 participant
Summary
Implementation details
Security improvements
Test plan
Fixes #11
Summary by CodeRabbit
New Features
Refactor
Tests
Chores