[#1] Implemented branch deployment feature.#8
Conversation
- Added functions to list branches from Git repository - Implemented branch deployment via Lagoon CLI - Added interactive menu option for branch deployment - Added autocomplete branch selection - Updated documentation
|
Warning Rate limit exceeded@richardgaunt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
""" WalkthroughA new interactive CLI feature for deploying Git branches to Lagoon environments was added. Supporting functions for fetching branches and deploying them were implemented, with robust input validation and error handling. Documentation was updated to describe the new feature, and dependencies and imports were adjusted to support branch autocomplete in prompts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LagoonAPI
participant GitRepo
User->>CLI: Select "Deploy Branch" in interactive mode
CLI->>LagoonAPI: Fetch project details (including Git URL)
CLI->>GitRepo: Fetch list of branches
GitRepo-->>CLI: Return branch list
CLI->>User: Prompt to select branch (autocomplete)
User->>CLI: Select branch
CLI->>User: Confirm deployment
User->>CLI: Confirm
CLI->>LagoonAPI: Initiate deployment for selected branch
LagoonAPI-->>CLI: Return deployment initiation status
CLI->>User: Show deployment status and instructions
Poem
✨ Finishing Touches
🪧 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: 4
🧹 Nitpick comments (3)
src/lagoon-api.js (2)
194-199: Usefs.mkdtemp()for safer, race-free temp-dir creation
path.join(os.tmpdir(), Date.now())can still collide under heavy concurrency and inherits OS permissions from the parent directory. Node already ships a purpose-built helper (fs.mkdtemp) that guarantees a unique, securely-permissioned directory.-async function createTempDir() { - const tempDir = path.join(os.tmpdir(), `lagoon-cli-wrapper-${Date.now()}`); - await fs.mkdir(tempDir, { recursive: true }); - return tempDir; -} +async function createTempDir() { + const prefix = path.join(os.tmpdir(), 'lagoon-cli-wrapper-'); + return await fs.mkdtemp(prefix); +}
232-265: Guarantee cleanup withfinallyinstead of nestedtry/catch
The nested structure works but is verbose and easy to miss additional early-returns in the future. Afinallyblock expresses intent and ensures the temp dir is deleted exactly once.- const tempDir = await createTempDir(); - - try { - await cloneRepository(gitUrl, tempDir); - const branches = await listBranches(tempDir); - await fs.rm(tempDir, { recursive: true, force: true }); - return branches; - } catch (error) { - try { - await fs.rm(tempDir, { recursive: true, force: true }); - } catch (cleanupError) { - console.error(`Failed to clean up temporary directory: ${cleanupError.message}`); - } - throw error; - } + const tempDir = await createTempDir(); + try { + await cloneRepository(gitUrl, tempDir); + return await listBranches(tempDir); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + }This reduces complexity and guarantees removal even if future
returnstatements are added.src/interactive.js (1)
493-507: Branch sorting logic could be simplified.While the sorting logic works as intended (prioritizing main/master/develop branches), it could be simplified using a more concise approach.
- const sortedBranches = branches.sort((a, b) => { - const priority = ['main', 'master', 'develop']; - const aIndex = priority.indexOf(a); - const bIndex = priority.indexOf(b); - - if (aIndex !== -1 && bIndex !== -1) { - return aIndex - bIndex; - } else if (aIndex !== -1) { - return -1; - } else if (bIndex !== -1) { - return 1; - } else { - return a.localeCompare(b); - } - }); + const priority = ['main', 'master', 'develop']; + const sortedBranches = branches.sort((a, b) => { + const aIndex = priority.indexOf(a); + const bIndex = priority.indexOf(b); + + // If both are priority branches, sort by priority order + // If only one is priority, it comes first + // Otherwise, sort alphabetically + return (aIndex !== -1 && bIndex !== -1) ? aIndex - bIndex : + aIndex !== -1 ? -1 : + bIndex !== -1 ? 1 : + a.localeCompare(b); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(3 hunks)package.json(1 hunks)src/interactive.js(4 hunks)src/lagoon-api.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lagoon-api.js (2)
src/logger.js (1)
logError(45-52)src/interactive.js (1)
branches(477-477)
🔇 Additional comments (9)
package.json (1)
27-29: Dependency added – runnpm audit& pin if vulnerable
inquirer-autocomplete-promptlooks fine, but please runnpm audit(orpnpm audit) to ensure no transitive vulnerabilities are introduced, and consider pinning to an exact version if reproducibility is critical.README.md (1)
98-108: Great documentation update 🎉
The new “Branch Deployment” section is clear, covers validation and asynchronous behaviour, and aligns well with the implemented API.src/interactive.js (7)
79-81: Properly integrated with the existing workflow.The new case for deployBranch is appropriately integrated with the existing switch statement and passes the correct parameters to the deployBranchFlow function.
183-183: Menu option correctly added.The Deploy Branch menu option is properly added to the main menu choices.
458-470: Good validation of Git URL requirement.The function correctly validates that the project has a Git URL configured before proceeding, providing clear feedback to the user when missing.
509-541: Good implementation of branch selection with autocomplete fallback.The implementation provides a smooth user experience with autocomplete for branch selection, with an appropriate fallback to a standard list when needed. The filtering for the autocomplete works well for partial matches.
543-557: User confirmation step enhances safety.Adding a confirmation step before deployment is a good security practice, especially with the default set to false to prevent accidental deployments.
558-570: Clear deployment feedback to users.The code provides appropriate feedback during and after deployment, including useful information about the asynchronous nature of the deployment process.
571-581: Good error handling and user continuation flow.The function properly handles errors during branch fetching and ensures the user can continue with the interactive flow regardless of the outcome.
src/lagoon-api.js
Outdated
| // Deploy a branch to Lagoon | ||
| export async function deployBranch(instance, project, branch) { | ||
| try { | ||
| // Validate branch name to prevent command injection | ||
| if (!/^[a-zA-Z0-9_.-]+$/.test(branch)) { | ||
| throw new Error('Invalid branch name. Branch names must contain only alphanumeric characters, underscores, hyphens, and periods.'); | ||
| } | ||
|
|
||
| const command = `lagoon -l ${instance} -p ${project} deploy branch --branch ${branch} --output-json`; | ||
| const { stdout } = await execLagoonCommand(command, `Deploy Branch ${branch} to ${project}`); | ||
|
|
||
| // Parse the JSON response | ||
| const response = JSON.parse(stdout); | ||
|
|
||
| if (response.result === 'success') { | ||
| return { | ||
| success: true, | ||
| message: `Branch ${branch} is being deployed to ${project}` | ||
| }; | ||
| } else { | ||
| throw new Error(`Failed to deploy branch ${branch}: ${JSON.stringify(response)}`); | ||
| } | ||
| } catch (error) { | ||
| throw new Error(`Failed to deploy branch ${branch}: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Branch-name regex too restrictive – blocks common names with /
Valid git branches routinely include slashes (feature/xyz). Current regex ^[a-zA-Z0-9_.-]+$ will reject them, limiting usability without adding meaningful protection (the value is still injected into a shell string).
Recommend:
- Allow forward slashes:
^[A-Za-z0-9_.\\/\\-]+$ - Escape all CLI arguments instead of whitelisting – use
spawn/execFileso each arg is passed verbatim, eliminating injection vectors frominstance,project, andbranch.
- if (!/^[a-zA-Z0-9_.-]+$/.test(branch)) {
+ if (!/^[A-Za-z0-9_.\\/\\-]+$/.test(branch)) {Long-term: switch the entire Lagoon call to spawn('lagoon', ['-l', instance, '-p', project, 'deploy', 'branch', '--branch', branch, '--output-json']).
📝 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.
| // Deploy a branch to Lagoon | |
| export async function deployBranch(instance, project, branch) { | |
| try { | |
| // Validate branch name to prevent command injection | |
| if (!/^[a-zA-Z0-9_.-]+$/.test(branch)) { | |
| throw new Error('Invalid branch name. Branch names must contain only alphanumeric characters, underscores, hyphens, and periods.'); | |
| } | |
| const command = `lagoon -l ${instance} -p ${project} deploy branch --branch ${branch} --output-json`; | |
| const { stdout } = await execLagoonCommand(command, `Deploy Branch ${branch} to ${project}`); | |
| // Parse the JSON response | |
| const response = JSON.parse(stdout); | |
| if (response.result === 'success') { | |
| return { | |
| success: true, | |
| message: `Branch ${branch} is being deployed to ${project}` | |
| }; | |
| } else { | |
| throw new Error(`Failed to deploy branch ${branch}: ${JSON.stringify(response)}`); | |
| } | |
| } catch (error) { | |
| throw new Error(`Failed to deploy branch ${branch}: ${error.message}`); | |
| } | |
| } | |
| // Deploy a branch to Lagoon | |
| export async function deployBranch(instance, project, branch) { | |
| try { | |
| // Validate branch name to prevent command injection | |
| - if (!/^[a-zA-Z0-9_.-]+$/.test(branch)) { | |
| + if (!/^[A-Za-z0-9_.\/\-]+$/.test(branch)) { | |
| throw new Error('Invalid branch name. Branch names must contain only alphanumeric characters, underscores, hyphens, and periods.'); | |
| } | |
| const command = `lagoon -l ${instance} -p ${project} deploy branch --branch ${branch} --output-json`; | |
| const { stdout } = await execLagoonCommand(command, `Deploy Branch ${branch} to ${project}`); | |
| // Parse the JSON response | |
| const response = JSON.parse(stdout); | |
| if (response.result === 'success') { | |
| return { | |
| success: true, | |
| message: `Branch ${branch} is being deployed to ${project}` | |
| }; | |
| } else { | |
| throw new Error(`Failed to deploy branch ${branch}: ${JSON.stringify(response)}`); | |
| } | |
| } catch (error) { | |
| throw new Error(`Failed to deploy branch ${branch}: ${error.message}`); | |
| } | |
| } |
There was a problem hiding this comment.
Create an issue for adding the long term fix - this will be need to implemented everywhere we call the lagoon command
There was a problem hiding this comment.
🧩 Analysis chain
@richardgaunt I've created a GitHub issue for implementing the long-term security fix using spawn/execFile for all Lagoon CLI commands. The issue includes:
- Description of the current security vulnerability
- Proposed solution with code examples
- Implementation plan
- Benefits of the new approach
- References to this PR discussion
I've assigned the issue to you as requested. This fix will indeed need to be implemented everywhere the Lagoon command is called to fully eliminate the command injection vulnerability.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the long-term fix
ISSUE_TITLE="Security Enhancement: Switch to spawn/execFile for Lagoon CLI command execution"
ISSUE_BODY="## Background
As identified in [PR #8](https://github.com/richardgaunt/lagoon-cli-wrapper/pull/8#discussion_r2077246796), the current approach of using string interpolation when executing Lagoon CLI commands creates potential command injection vulnerabilities.
## Problem
Currently, command arguments (like instance names, project names, branch names, etc.) are interpolated directly into shell command strings:
\`\`\`javascript
const command = \`lagoon -l \${instance} -p \${project} deploy branch --branch \${branch} --output-json\`;
const { stdout } = await execLagoonCommand(command, \`Deploy Branch \${branch} to \${project}\`);
\`\`\`
While we've implemented regex validation in some places, this approach is still vulnerable to command injection if an argument contains shell metacharacters.
## Proposed Solution
Replace string interpolation with Node.js \`spawn\` or \`execFile\` functions to pass arguments securely:
\`\`\`javascript
const { stdout } = await util.promisify(child_process.execFile)('lagoon', [
'-l', instance,
'-p', project,
'deploy', 'branch',
'--branch', branch,
'--output-json'
]);
\`\`\`
Or using spawn with proper output handling:
\`\`\`javascript
const lagoonProcess = child_process.spawn('lagoon', [
'-l', instance,
'-p', project,
'deploy', 'branch',
'--branch', branch,
'--output-json'
]);
// Handle output streams appropriately
\`\`\`
## Implementation Plan
1. Create a new secure wrapper function for Lagoon CLI calls
2. Refactor all instances of \`execLagoonCommand\` to use the new wrapper
3. Update the logging mechanism to work with the new approach
4. Add tests to verify the security of the implementation
## Benefits
- Eliminates command injection vulnerabilities
- More maintainable and secure code
- No need for complex regex validation of input parameters
- Explicit separation of command and arguments improves readability
## Related
- PR #8: Implemented branch deployment feature
- Discussion: https://github.com/richardgaunt/lagoon-cli-wrapper/pull/8#discussion_r2077246796
cc @richardgaunt"
# Create the issue using GitHub CLI
gh issue create --repo richardgaunt/lagoon-cli-wrapper --title "$ISSUE_TITLE" --body "$ISSUE_BODY" --assignee richardgaunt
Length of output: 4613
@richardgaunt I've successfully created issue #11 titled "Security Enhancement: Switch to spawn/execFile for Lagoon CLI command execution" in your repository.
The issue includes detailed information about:
- The current security vulnerability
- Code examples showing the problem
- The proposed solution using
spawn/execFile - A clear implementation plan
- Benefits of making the security enhancement
You can view the issue here: #11
✏️ Learnings added
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/interactive.mjs (1)
20-26:autocompletefallback is fine, but you forgot to record availabilityWhen the dynamic import fails you log a warning, yet later code still assumes the prompt type exists (see next comment).
Consider setting a boolean flag, e.g.const hasAutocompletePrompt = true/false, to drive the prompt selection logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
index.mjs(1 hunks)src/interactive.mjs(5 hunks)src/lagoon-api.mjs(4 hunks)src/lagoon-ssh-key-configurator.mjs(1 hunks)src/logger.mjs(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/lagoon-ssh-key-configurator.mjs
- index.mjs
- src/logger.mjs
🧰 Additional context used
🪛 ESLint
src/interactive.mjs
[error] 16-16: require file extension '.mjs'.
(n/file-extension-in-import)
[error] 17-17: require file extension '.mjs'.
(n/file-extension-in-import)
[error] 18-18: require file extension '.mjs'.
(n/file-extension-in-import)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/lagoon-api.mjs (3)
210-258:⚠️ Potential issueCommand injection vulnerability in getGitBranches.
The function directly interpolates user input into a shell command without proper escaping, creating a security risk.
Replace the current implementation with a safer approach that uses
execFileto avoid shell parsing:-const command = `git ls-remote --heads ${gitUrl}`; -console.log(chalk.blue(`Executing: ${chalk.bold(command)}`)); - -try { - const { stdout, stderr } = await execAsync(command); +console.log(chalk.blue(`Executing: ${chalk.bold(`git ls-remote --heads ${gitUrl}`)}`)); + +try { + // Use execFile (promisified) to avoid shell parsing and lift the buffer cap + const execFileAsync = promisify(require('child_process').execFile); + const { stdout, stderr } = await execFileAsync('git', ['ls-remote', '--heads', gitUrl], { + maxBuffer: 1024 * 1024 * 10 // 10 MB, adjust as needed + });This approach prevents command injection by separating the command from its arguments and increases the buffer size to handle repositories with many branches.
262-267:⚠️ Potential issueOver-restrictive branch name validation.
The current regex for branch validation is too restrictive and will reject valid Git branch names (like 'feature/xyz' or branches with '@' characters).
Modify the branch name validation to allow common branch naming patterns while still preventing command injection:
-if (!/^[a-zA-Z0-9_.-]+$/.test(branch)) { +// Allow slashes, at-signs, and other characters common in Git branch names +if (!/^[\w.@/-]+$/.test(branch)) { throw new Error('Invalid branch name. Branch names must contain only alphanumeric characters, underscores, hyphens, and periods.'); }
268-268: 🛠️ Refactor suggestionMissing quotes around branch name in command.
Branch names with spaces or special characters might cause shell interpretation issues.
Quote the branch parameter to handle branches with spaces or special characters:
-const command = `lagoon -l ${instance} -p ${project} deploy branch --branch ${branch} --output-json`; +const command = `lagoon -l ${instance} -p ${project} deploy branch --branch "${branch}" --output-json`;
🧹 Nitpick comments (1)
src/lagoon-api.mjs (1)
207-258: Consider adding error handling for network timeouts.The current implementation handles authentication errors and "not found" errors, but network timeouts or connection issues might need specific handling too.
Add a timeout option and handle network-related errors:
try { // Use execFile (promisified) to avoid shell parsing const execFileAsync = promisify(require('child_process').execFile); const { stdout, stderr } = await execFileAsync('git', ['ls-remote', '--heads', gitUrl], { maxBuffer: 1024 * 1024 * 10, // 10 MB, adjust as needed + timeout: 30000 // 30 seconds timeout }); // Rest of the function... } catch (error) { // If the Git URL is using SSH, offer a hint about authentication if (gitUrl.startsWith('git@') && error.message.includes('Permission denied')) { throw new Error(`Authentication failed for ${gitUrl}. Please check your SSH key configuration.`); } else if (error.message.includes('not found')) { throw new Error(`Repository not found: ${gitUrl}. Please check if the URL is correct.`); + } else if (error.code === 'ETIMEDOUT' || error.code === 'ECONNREFUSED') { + throw new Error(`Network error when accessing ${gitUrl}. Please check your internet connection and that the repository is accessible.`); } else { throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/interactive.mjs(5 hunks)src/lagoon-api.mjs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interactive.mjs
🔇 Additional comments (5)
src/lagoon-api.mjs (5)
4-4: Clean import statement update.The explicit import path './logger.mjs' improves code clarity and maintainability.
117-128: Great addition of JSDoc documentation.The comprehensive JSDoc comments for the
deleteEnvironmentfunction significantly improve code understanding by clearly documenting parameters, return values, and potential error cases.
172-177: Well-structured JSDoc for gitUrlToGithubUrl.The documentation is clear and complete, with proper parameter and return type annotations.
270-284: Good error handling and response parsing.The function properly parses the JSON response and provides clear error messages when deployment fails.
229-239: Good branch name extraction logic.The code correctly parses the output of
git ls-remoteand filters out invalid entries.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lagoon-api.mjs (2)
209-258:⚠️ Potential issueCommand injection & buffer overflow risk in
getGitBranches()The current implementation has security vulnerabilities that were previously identified but not fully addressed:
- Command injection risk:
gitUrlis inserted directly into a shell command without proper sanitization- Memory limitation: Using
execbuffers all output in memory which could cause issues with large repositoriesApply this fix to address both security concerns:
- const command = `git ls-remote --heads ${gitUrl}`; - console.log(chalk.blue(`Executing: ${chalk.bold(command)}`)); - - try { - const { stdout, stderr } = await execAsync(command); + console.log(chalk.blue(`Executing: ${chalk.bold('git ls-remote --heads')} for ${gitUrl}`)); + + try { + // Use child_process.execFile instead of exec to prevent command injection + const execFileAsync = promisify(require('child_process').execFile); + const { stdout, stderr } = await execFileAsync('git', ['ls-remote', '--heads', gitUrl], { + maxBuffer: 1024 * 1024 * 10 // 10 MB buffer to handle repositories with many branches + });Additionally, consider using
execLagoonCommandwith a modified version that accepts array arguments for better consistency with the rest of the codebase.
264-265:⚠️ Potential issueFix regex pattern for branch name validation
The current regex pattern is too restrictive for valid Git branch names and unnecessarily escapes the hyphen character.
- if (!/^[a-zA-Z0-9_./\-]+$/.test(branch)) { + // Allow common Git branch name characters including slashes, at-signs, etc. while still blocking potential injection characters + if (!/^[\w./\-@]+$/.test(branch)) {The updated regex uses
\w(which includes alphanumeric characters plus underscore) and adds@to support additional common branch name patterns while still preventing command injection.🧰 Tools
🪛 ESLint
[error] 264-264: Unnecessary escape character: -.
(no-useless-escape)
🧹 Nitpick comments (2)
src/lagoon-api.mjs (2)
207-208: Consider clarifying or removing this comment.This comment doesn't provide much context or value. Either expand on why no helper functions are needed for the git ls-remote approach or remove it to improve code clarity.
260-288: Consider adding JSDoc for the new functionsBoth
getGitBranchesanddeployBranchwould benefit from JSDoc documentation similar to what you've added for the other functions, detailing parameters, return values, and potential errors.🧰 Tools
🪛 ESLint
[error] 264-264: Unnecessary escape character: -.
(no-useless-escape)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lagoon-api.mjs(4 hunks)
🧰 Additional context used
🪛 ESLint
src/lagoon-api.mjs
[error] 264-264: Unnecessary escape character: -.
(no-useless-escape)
🔇 Additional comments (3)
src/lagoon-api.mjs (3)
117-128: Excellent addition of comprehensive JSDoc docs.The detailed documentation for the
deleteEnvironmentfunction is clear and well-structured, properly documenting parameters, return values, and error conditions.
172-177: Thorough JSDoc documentation added.The JSDoc for
gitUrlToGithubUrlis well-written and clearly explains the function's purpose, parameters, and return values.
273-284: Good job handling the deploy response and providing clear feedback.The function properly parses the JSON response, checks for success, and returns a meaningful message to the user.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lagoon-api.mjs (2)
264-266: Consider allowing more Git branch name charactersGit branches can contain more characters than currently allowed, such as
@signs which are common in CI-generated branches.- if (!/^[a-zA-Z0-9_./\-]+$/.test(branch)) { + // Allow slashes, at-signs, and common Git branch characters while forbidding whitespace & control chars + if (!/^[\w.@/-]+$/.test(branch)) { throw new Error('Invalid branch name. Branch names must contain only alphanumeric characters, slashes, underscores, hyphens, and periods.'); }This keeps the shell safe while allowing more common branch naming conventions.
🧰 Tools
🪛 ESLint
[error] 264-264: Unnecessary escape character: -.
(no-useless-escape)
209-258:⚠️ Potential issueCommand injection vulnerability in
getGitBranches()The function still uses
execwith string interpolation which creates a command injection risk. A malicious URL could execute arbitrary commands.Replace the current implementation with
execFileto avoid shell parsing:- const command = `git ls-remote --heads ${gitUrl}`; - console.log(chalk.blue(`Executing: ${chalk.bold(command)}`)); - - try { - const { stdout, stderr } = await execAsync(command); + console.log(chalk.blue(`Executing: ${chalk.bold(`git ls-remote --heads ${gitUrl}`)}`)); + + try { + // Use execFile (via promisify) to avoid shell parsing + const execFileAsync = promisify(require('child_process').execFile); + const { stdout, stderr } = await execFileAsync('git', ['ls-remote', '--heads', gitUrl], { + maxBuffer: 1024 * 1024 * 10 // 10 MB, adjust as needed + });Additionally, this addresses the potential memory issue, as repositories with many branches could exceed the default 200KB buffer.
🧹 Nitpick comments (3)
src/lagoon-api.mjs (3)
264-264: Fix unnecessary escape character in regexThe hyphen doesn't need to be escaped in this character class position.
- if (!/^[a-zA-Z0-9_./\-]+$/.test(branch)) { + if (!/^[a-zA-Z0-9_.\/-]+$/.test(branch)) {🧰 Tools
🪛 ESLint
[error] 264-264: Unnecessary escape character: -.
(no-useless-escape)
273-278: Consider error handling for non-JSON responsesIf the Lagoon CLI fails in a way that doesn't produce valid JSON output, the
JSON.parsewill throw an error that might be confusing.- const { stdout } = await execLagoonCommand(command, `Deploy Branch ${branch} to ${project}`); - - // Parse the JSON response - const response = JSON.parse(stdout); + const { stdout } = await execLagoonCommand(command, `Deploy Branch ${branch} to ${project}`); + + // Parse the JSON response + let response; + try { + response = JSON.parse(stdout); + } catch (parseError) { + throw new Error(`Failed to parse Lagoon response: ${stdout.substring(0, 100)}... (Response was not valid JSON)`); + }
210-244: Add handling for empty branch listsThe function could return an empty array if no branches are found. Consider adding a check and providing a more informative message to the user.
// Log the number of branches found console.log(chalk.green(`Found ${branches.length} branches in repository`)); + // If no branches were found, this might indicate an issue + if (branches.length === 0) { + console.log(chalk.yellow(`Warning: No branches found in repository. This might indicate an issue with the repository URL or permissions.`)); + } + return branches;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lagoon-api.mjs(4 hunks)
🧰 Additional context used
🪛 ESLint
src/lagoon-api.mjs
[error] 264-264: Unnecessary escape character: -.
(no-useless-escape)
🔇 Additional comments (3)
src/lagoon-api.mjs (3)
117-128: Good addition of comprehensive JSDoc comments!The JSDoc comments for the
deleteEnvironmentfunction are thorough and clearly document the function's purpose, parameters, return values, and possible error conditions.
172-177: Well-documented function with clear JSDocThe JSDoc for
gitUrlToGithubUrlprovides clear documentation about the function's purpose and behavior.
268-272: Good implementation of shell escapingThe comprehensive escaping for shell safety is a good implementation that addresses previous feedback.
Docstrings generation was requested by @richardgaunt. * #8 (comment) The following files were modified: * `src/interactive.mjs` * `src/lagoon-api.mjs` * `src/logger.mjs`
|
Note Generated docstrings for this pull request at #10 |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Checklist before requesting a review
[#1]Verb in past tense with dot at the end.Changedsection about WHY something was done if this was not a normal implementationChanged
This implementation allows users to deploy any branch from a project's Git repository to Lagoon. It uses a secure approach to clone and list branches, with careful validation to prevent command injection.
Summary by CodeRabbit
New Features
Documentation
Chores