Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ boundaries:

Rules apply to all tasks (single or PRD).

Config safety behavior:
- Invalid config files fall back to defaults with a warning.
- Unsafe prototype-pollution keys are rejected during config parsing.

## AI Engines

```bash
Expand Down
6 changes: 4 additions & 2 deletions cli/src/config/detector.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { existsSync, readFileSync } from "node:fs";
import { basename, join } from "node:path";
import { logDebug } from "../ui/logger.ts";

interface DetectedProject {
name: string;
Expand Down Expand Up @@ -106,8 +107,9 @@ function detectNodeProject(packageJsonPath: string, result: DetectedProject): vo
if (scripts.build) {
result.buildCmd = "npm run build";
}
} catch {
// Ignore parsing errors
} catch (error) {
// BUG FIX: Log parsing errors instead of silently ignoring
logDebug(`Failed to parse package.json: ${error}`);
}
}

Expand Down
4 changes: 2 additions & 2 deletions cli/src/config/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export * from "./types.ts";
export * from "./loader.ts";
export * from "./detector.ts";
export * from "./loader.ts";
export * from "./types.ts";
export * from "./writer.ts";
89 changes: 83 additions & 6 deletions cli/src/config/loader.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,58 @@
import { existsSync, readFileSync } from "node:fs";
import { join } from "node:path";
import YAML from "yaml";
import { validateCommand } from "../engines/validation.ts";
import { logDebug, logWarn } from "../ui/logger.ts";
import { type RalphyConfig, RalphyConfigSchema } from "./types.ts";

export const RALPHY_DIR = ".ralphy";

/**
* Recursively check for prototype pollution keys in parsed data
* BUG FIX: Uses recursive traversal instead of string matching to prevent Unicode escape bypasses
*/
function hasPrototypePollution(obj: unknown): boolean {
const MAX_DEPTH = 20;
const MAX_NODES = 10000;
const dangerousKeys = new Set(["__proto__", "constructor", "prototype"]);

if (typeof obj !== "object" || obj === null) return false;

const visited = new Set<unknown>();
const queue: Array<{ value: unknown; depth: number }> = [{ value: obj, depth: 0 }];
let nodesVisited = 0;

while (queue.length > 0) {
const current = queue.shift();
if (!current) continue;

nodesVisited++;
if (nodesVisited > MAX_NODES) {
throw new Error("Config file too complex to validate safely");
}

if (current.depth > MAX_DEPTH) {
throw new Error("Config file nesting exceeds safety limits");
}

if (typeof current.value !== "object" || current.value === null) {
continue;
}

if (visited.has(current.value)) {
continue;
}
visited.add(current.value);

for (const key of Object.keys(current.value)) {
if (dangerousKeys.has(key)) return true;
const value = (current.value as Record<string, unknown>)[key];
queue.push({ value, depth: current.depth + 1 });
}
}

return false;
}
export const CONFIG_FILE = "config.yaml";
export const PROGRESS_FILE = "progress.txt";

Expand Down Expand Up @@ -48,10 +97,17 @@ export function loadConfig(workDir = process.cwd()): RalphyConfig | null {
try {
const content = readFileSync(configPath, "utf-8");
const parsed = YAML.parse(content);

// BUG FIX: Proper prototype pollution protection with recursive check
// The old string-based check was bypassable via Unicode escapes
if (hasPrototypePollution(parsed)) {
throw new Error("Config file contains potentially malicious prototype pollution keys");
}

return RalphyConfigSchema.parse(parsed);
} catch (error) {
// Log error for debugging, but return default config
console.error(`Warning: Failed to parse config at ${configPath}:`, error);
logWarn(`Invalid config file at ${configPath}: ${error}. Falling back to defaults.`);
logDebug(`Config parse details: ${error}`);
return RalphyConfigSchema.parse({});
}
}
Expand All @@ -69,31 +125,52 @@ export function loadRules(workDir = process.cwd()): string[] {
*/
export function loadBoundaries(workDir = process.cwd()): string[] {
const config = loadConfig(workDir);
return config?.boundaries.never_touch ?? [];
return config?.boundaries?.never_touch ?? [];
}

/**
* Get test command from config
*/
export function loadTestCommand(workDir = process.cwd()): string {
const config = loadConfig(workDir);
return config?.commands.test ?? "";
const command = config?.commands.test ?? "";

if (command && !validateCommand(command)) {
logWarn(`Invalid test command in config: "${command}". Falling back to default.`);
return "";
}

return command;
}

/**
* Get lint command from config
*/
export function loadLintCommand(workDir = process.cwd()): string {
const config = loadConfig(workDir);
return config?.commands.lint ?? "";
const command = config?.commands.lint ?? "";

if (command && !validateCommand(command)) {
logWarn(`Invalid lint command in config: "${command}". Falling back to default.`);
return "";
}

return command;
Comment on lines 159 to +168

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium config/loader.ts:149

Suggestion: validateCommand forbids spaces, so multi-word commands in loadTestCommand, loadLintCommand, and loadBuildCommand are treated as invalid and reset to empty strings. Consider splitting the config into command and args and validate only the executable, or relax the validation to allow spaces/common arg syntax to avoid rejecting valid commands.

-	if (command && !validateCommand(command)) {
+	if (command && !validateCommand(command.split(' ')[0])) {
		logWarn(`Invalid lint command in config: "${command}". Falling back to default.`);
		return "";
	}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/config/loader.ts around lines 149-158:

Suggestion: `validateCommand` forbids spaces, so multi-word commands in `loadTestCommand`, `loadLintCommand`, and `loadBuildCommand` are treated as invalid and reset to empty strings. Consider splitting the config into `command` and `args` and validate only the executable, or relax the validation to allow spaces/common arg syntax to avoid rejecting valid commands.

Evidence trail:
cli/src/engines/validation.ts lines 50-56: validateCommand uses regex `/^[a-zA-Z0-9._\-/]+$/` which does not include space character. cli/src/config/loader.ts lines 127-139 (loadTestCommand), 149-161 (loadLintCommand), 166-178 (loadBuildCommand): all call validateCommand and return empty string "" when validation fails.

}

/**
* Get build command from config
*/
export function loadBuildCommand(workDir = process.cwd()): string {
const config = loadConfig(workDir);
return config?.commands.build ?? "";
const command = config?.commands.build ?? "";

if (command && !validateCommand(command)) {
logWarn(`Invalid build command in config: "${command}". Falling back to default.`);
return "";
}

return command;
}

/**
Expand Down
68 changes: 13 additions & 55 deletions cli/src/config/writer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs";
import { appendFile } from "node:fs/promises";
import { appendFileSync, existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs";
import YAML from "yaml";
import { detectProject } from "./detector.ts";
import { getConfigPath, getProgressPath, getRalphyDir } from "./loader.ts";
Expand Down Expand Up @@ -35,7 +34,7 @@ rules:
# - "Use server actions instead of API routes in Next.js"
#
# Skills/playbooks (optional):
# - "Before coding, read and follow any relevant skill/playbook docs under .opencode/skills, .claude/skills, or .github/skills."
# - "Before coding, read and follow any relevant skill/playbook docs under .opencode/skills or .claude/skills."

# Boundaries - files/folders the AI should not modify
boundaries:
Expand All @@ -49,9 +48,15 @@ boundaries:

/**
* Escape a value for safe YAML string
* BUG FIX: Use YAML library for proper escaping to prevent injection attacks
*/
function escapeYaml(value: string | undefined | null): string {
return (value || "").replace(/"/g, '\\"');
if (!value) return "";
// Use YAML library for proper escaping instead of simple quote replacement
// This prevents YAML injection attacks
const serialized = YAML.stringify(value).trim();
// Remove surrounding quotes added by YAML.stringify for simple strings
return serialized.replace(/^"|"$/g, "");
}
Comment on lines 55 to 64

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High config/writer.ts:53

escapeYaml now uses YAML.stringify, which can return single-quoted strings (e.g., 'foo"bar') or block scalars. createConfigContent wraps this output in double quotes, so a command like echo "hello" becomes name: "'echo \"hello\"'" — a string literal containing single quotes that corrupts the value when parsed. For multi-line strings, wrapping a block scalar in quotes produces invalid YAML.

 function escapeYaml(value: string | undefined | null): string {
 	if (!value) return "";
-	// Use YAML library for proper escaping instead of simple quote replacement
-	// This prevents YAML injection attacks
-	const serialized = YAML.stringify(value).trim();
-	// Remove surrounding quotes added by YAML.stringify for simple strings
-	return serialized.replace(/^"|"$/g, "");
+	// Escape backslashes and double quotes, then wrap in double quotes
+	const escaped = value.replace(/\\/g, "\\\\").replace(/"/g, "\\\"");
+	return `"${escaped}"`;
 }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/config/writer.ts around lines 53-60:

`escapeYaml` now uses `YAML.stringify`, which can return single-quoted strings (e.g., `'foo"bar'`) or block scalars. `createConfigContent` wraps this output in double quotes, so a command like `echo "hello"` becomes `name: "'echo \"hello\"'"` — a string literal containing single quotes that corrupts the value when parsed. For multi-line strings, wrapping a block scalar in quotes produces invalid YAML.

Evidence trail:
cli/src/config/writer.ts lines 50-57: `escapeYaml` function uses `YAML.stringify(value).trim()` and strips only double quotes with `replace(/^"|"/g, "")`. Single quotes and block scalars are not handled.

cli/src/config/writer.ts lines 15-21: `createConfigContent` wraps `escapeYaml` output in double quotes, e.g., `name: "${escapeYaml(detected.name)}"` and `test: "${escapeYaml(detected.testCmd)}"`.

When `YAML.stringify` returns single-quoted strings (for inputs containing double quotes) or block scalars (for multi-line strings), the wrapping in double quotes produces invalid YAML.


/**
Expand Down Expand Up @@ -108,44 +113,8 @@ export function addRule(rule: string, workDir = process.cwd()): void {
writeFileSync(configPath, YAML.stringify(parsed), "utf-8");
}

/** Queue for batching progress writes */
const progressWriteQueue: Map<string, string[]> = new Map();
let flushTimeout: ReturnType<typeof setTimeout> | null = null;

/**
* Flush all pending progress writes to disk
*/
async function flushProgressWrites(): Promise<void> {
if (progressWriteQueue.size === 0) return;

const entries = [...progressWriteQueue.entries()];
progressWriteQueue.clear();
flushTimeout = null;

for (const [path, lines] of entries) {
try {
await appendFile(path, lines.join(""), "utf-8");
} catch {
// Ignore write errors for progress logging
}
}
}

/**
* Schedule a flush of progress writes (debounced)
*/
function scheduleFlush(): void {
if (flushTimeout) return;
flushTimeout = setTimeout(() => {
void flushProgressWrites();
}, 100); // Batch writes within 100ms window
}

/**
* Log a task to the progress file (async, batched)
*
* Performance optimized: uses async I/O and batches writes within 100ms windows
* to reduce file system contention in parallel mode.
* Log a task to the progress file
*/
export function logTaskProgress(
task: string,
Expand All @@ -162,23 +131,12 @@ export function logTaskProgress(
const icon = status === "completed" ? "✓" : "✗";
const line = `- [${icon}] ${timestamp} - ${task}\n`;

// Add to write queue
const existing = progressWriteQueue.get(progressPath) || [];
existing.push(line);
progressWriteQueue.set(progressPath, existing);

// Schedule async flush
scheduleFlush();
appendFileSync(progressPath, line, "utf-8");
}

/**
* Force flush all pending progress writes immediately
* Call this before process exit to ensure all writes are persisted
* @deprecated Writes are synchronous; kept for API compatibility.
*/
export async function flushAllProgressWrites(): Promise<void> {
if (flushTimeout) {
clearTimeout(flushTimeout);
flushTimeout = null;
}
await flushProgressWrites();
return Promise.resolve();
}
Loading