Merged
Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontends/dashboard/src/utils/process.ts`:
- Around line 7-10: The helper shellEscape is fine but runCommand currently
accepts raw shell strings allowing command injection; change runCommand to use
argv-style execution (e.g., execFile/spawn with an args array) instead of
interpolating into a shell, update callers that build shell templates
(references: runCommand, shellEscape and the helm-related callsites that
interpolate namespace/releaseName/image/secretName and the home callsite that
interpolates name/namespace) to pass each token as a separate argument rather
than embedding them in a shell string, and remove reliance on manual shellEscape
at those callsites so all command construction uses safe argv parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e941cced-ff10-4ec4-aa47-aaa6ae76ab50
📒 Files selected for processing (10)
extensions/apex-fusion/values.yamlextensions/cardano-node/values.yamlextensions/dolos/values.yamlextensions/hydra-node/values.yamlextensions/midnight/values.yamlfrontends/dashboard/src/utils/helm-install/dolos.tsfrontends/dashboard/src/utils/helm-install/index.tsfrontends/dashboard/src/utils/helm-install/midnight.tsfrontends/dashboard/src/utils/home/calls.tsfrontends/dashboard/src/utils/process.ts
Comment on lines
+7
to
+10
| export function shellEscape(value: string): string { | ||
| const quote = '\''; | ||
| return quote + value.replace(/'/g, `${quote}\\${quote}${quote}`) + quote; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find runCommand template literals with interpolations not wrapped by shellEscape(...)
python - <<'PY'
import pathlib, re
for p in pathlib.Path('.').rglob('*.ts'):
txt = p.read_text(encoding='utf-8', errors='ignore')
for m in re.finditer(r'runCommand\(`([\s\S]*?)`\.trim\(\)\)', txt):
cmd = m.group(1)
if '${' not in cmd:
continue
exprs = re.findall(r'\$\{([^}]*)\}', cmd)
bad = [e.strip() for e in exprs if not e.strip().startswith('shellEscape(')]
if bad:
line = txt.count('\n', 0, m.start()) + 1
print(f"{p}:{line} -> {bad}")
PY
# Scope where shell-based exec is used
rg -nP --type=ts '\bexec\(' -C2Repository: txpipe/metis
Length of output: 930
🏁 Script executed:
# Check if helm.ts exists and inspect the mentioned lines
fd -t f "helm.ts" 2>/dev/null | head -5
# Check the exact location in midnight.ts
cat -n frontends/dashboard/src/utils/helm-install/midnight.ts 2>/dev/null | sed -n '20,35p'
# Check home/calls.ts at the mentioned line
cat -n frontends/dashboard/src/utils/home/calls.ts 2>/dev/null | sed -n '325,340p'
# Verify the shellEscape function is correctly implemented
cat -n frontends/dashboard/src/utils/process.ts 2>/dev/null | head -20Repository: txpipe/metis
Length of output: 1989
🏁 Script executed:
# Check the full helm.ts file for lines around 126-139
wc -l frontends/dashboard/src/utils/helm.ts
# Show lines 120-145 if they exist
cat -n frontends/dashboard/src/utils/helm.ts 2>/dev/null | sed -n '120,145p'Repository: txpipe/metis
Length of output: 935
Shell escaping is incomplete; multiple callsites have unescaped interpolations that expose command injection.
The shellEscape() helper is correctly implemented, but runCommand() still accepts raw shell strings—making safety dependent on every caller remembering to escape. Current code has unescaped variables at:
frontends/dashboard/src/utils/helm.ts:126-127(${namespace}in template)frontends/dashboard/src/utils/helm.ts:139(${releaseName},${image},${namespace})frontends/dashboard/src/utils/helm.ts:30(${secretName}in helm set)frontends/dashboard/src/utils/home/calls.ts:332(${name},${namespace})
Prefer argv-based execution (execFile/spawn) to eliminate interpolation risks entirely—this prevents the class of errors already present in the codebase.
Proposed hardening direction (root-cause fix)
import { promisify } from 'util';
import childProcess from 'child_process';
const exec = promisify(childProcess.exec);
+const execFile = promisify(childProcess.execFile);
const DEFAULT_TIMEOUT = 30000; // 30 seconds
export function shellEscape(value: string): string {
const quote = '\'';
return quote + value.replace(/'/g, `${quote}\\${quote}${quote}`) + quote;
}
+export async function runCommandArgs(command: string, args: string[]): Promise<string> {
+ const { stdout, stderr } = await execFile(command, args, { timeout: DEFAULT_TIMEOUT });
+ if (stderr && !stderr.toLowerCase().includes('warning')) {
+ throw new Error(stderr);
+ }
+ return stdout;
+}
+
export async function runCommand(command: string): Promise<string> {
const { stdout, stderr } = await exec(command, { timeout: DEFAULT_TIMEOUT });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontends/dashboard/src/utils/process.ts` around lines 7 - 10, The helper
shellEscape is fine but runCommand currently accepts raw shell strings allowing
command injection; change runCommand to use argv-style execution (e.g.,
execFile/spawn with an args array) instead of interpolating into a shell, update
callers that build shell templates (references: runCommand, shellEscape and the
helm-related callsites that interpolate namespace/releaseName/image/secretName
and the home callsite that interpolates name/namespace) to pass each token as a
separate argument rather than embedding them in a shell string, and remove
reliance on manual shellEscape at those callsites so all command construction
uses safe argv parameters.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
displayNameoption across multiple Helm charts for custom display name overrides.Security