mirror of https://github.com/razor-ai/soup.git
fix: strengthen path confinement in generate command (security review)
Replace simple '..' check with resolve() + relative_to(cwd) for output path. Add same confinement guard to --seed, --dedup-with, and --context file paths. Add _path_within_cwd helper. 4 new security tests.
This commit is contained in:
parent
ea8f785b50
commit
5ecfb0b29c
|
|
@ -194,10 +194,15 @@ def generate(
|
|||
filter_output = True
|
||||
dedup_output = True
|
||||
|
||||
cwd = Path.cwd().resolve()
|
||||
|
||||
# Load seed examples if provided
|
||||
seed_examples = []
|
||||
if seed_file:
|
||||
seed_path = Path(seed_file)
|
||||
seed_path = Path(seed_file).resolve()
|
||||
if not _path_within_cwd(seed_path, cwd):
|
||||
console.print("[red]Seed file must be within the current directory[/]")
|
||||
raise typer.Exit(1)
|
||||
if not seed_path.exists():
|
||||
console.print(f"[red]Seed file not found: {seed_path}[/]")
|
||||
raise typer.Exit(1)
|
||||
|
|
@ -209,7 +214,10 @@ def generate(
|
|||
# Load existing data for dedup
|
||||
existing_texts = set()
|
||||
if dedup_with:
|
||||
dedup_path = Path(dedup_with)
|
||||
dedup_path = Path(dedup_with).resolve()
|
||||
if not _path_within_cwd(dedup_path, cwd):
|
||||
console.print("[red]Dedup file must be within the current directory[/]")
|
||||
raise typer.Exit(1)
|
||||
if not dedup_path.exists():
|
||||
console.print(f"[red]Dedup file not found: {dedup_path}[/]")
|
||||
raise typer.Exit(1)
|
||||
|
|
@ -223,7 +231,10 @@ def generate(
|
|||
# Load template context if provided
|
||||
context_text = ""
|
||||
if template_context:
|
||||
ctx_path = Path(template_context)
|
||||
ctx_path = Path(template_context).resolve()
|
||||
if not _path_within_cwd(ctx_path, cwd):
|
||||
console.print("[red]Context file must be within the current directory[/]")
|
||||
raise typer.Exit(1)
|
||||
if not ctx_path.exists():
|
||||
console.print(f"[red]Context file not found: {ctx_path}[/]")
|
||||
raise typer.Exit(1)
|
||||
|
|
@ -294,9 +305,9 @@ def generate(
|
|||
progress.update(task, advance=generated_this_round)
|
||||
|
||||
# Sanitize output path (prevent path traversal)
|
||||
out_path = Path(output)
|
||||
if ".." in out_path.parts:
|
||||
console.print("[red]Output path must not contain '..'[/]")
|
||||
out_path = Path(output).resolve()
|
||||
if not _path_within_cwd(out_path, cwd):
|
||||
console.print("[red]Output path must be within the current directory[/]")
|
||||
raise typer.Exit(1)
|
||||
|
||||
# Write output
|
||||
|
|
@ -906,6 +917,15 @@ def _validate_preference(example: dict) -> bool:
|
|||
return False
|
||||
|
||||
|
||||
def _path_within_cwd(path: Path, cwd: Path) -> bool:
|
||||
"""Check that a resolved path is within the current working directory."""
|
||||
try:
|
||||
path.relative_to(cwd)
|
||||
return True
|
||||
except ValueError:
|
||||
return False
|
||||
|
||||
|
||||
def _row_to_text(row: dict) -> str:
|
||||
"""Convert a row to a text string for dedup comparison."""
|
||||
return " ".join(str(v) for v in row.values() if v)
|
||||
|
|
|
|||
|
|
@ -973,6 +973,42 @@ class TestValidatePreference:
|
|||
# ─── Output Path Sanitization Tests ─────────────────────────────────────
|
||||
|
||||
|
||||
class TestPathWithinCwd:
|
||||
"""Test _path_within_cwd helper."""
|
||||
|
||||
def test_path_within_cwd(self, tmp_path):
|
||||
"""Path inside cwd should return True."""
|
||||
|
||||
from soup_cli.commands.generate import _path_within_cwd
|
||||
|
||||
cwd = tmp_path.resolve()
|
||||
child = (tmp_path / "subdir" / "file.jsonl").resolve()
|
||||
assert _path_within_cwd(child, cwd) is True
|
||||
|
||||
def test_path_outside_cwd(self, tmp_path):
|
||||
"""Path outside cwd should return False."""
|
||||
|
||||
from soup_cli.commands.generate import _path_within_cwd
|
||||
|
||||
cwd = (tmp_path / "subdir").resolve()
|
||||
outside = tmp_path.resolve()
|
||||
assert _path_within_cwd(outside, cwd) is False
|
||||
|
||||
def test_absolute_path_outside_cwd(self):
|
||||
"""Absolute path to system directory should return False."""
|
||||
from pathlib import Path
|
||||
|
||||
from soup_cli.commands.generate import _path_within_cwd
|
||||
|
||||
cwd = Path.cwd().resolve()
|
||||
# /tmp or C:\Windows are outside typical cwd
|
||||
system_path = Path("/tmp/exfil.jsonl").resolve()
|
||||
# This may or may not be within cwd depending on where tests run,
|
||||
# but the function itself should work correctly
|
||||
result = _path_within_cwd(system_path, cwd)
|
||||
assert isinstance(result, bool)
|
||||
|
||||
|
||||
class TestOutputPathSanitization:
|
||||
"""Test that output paths are sanitized."""
|
||||
|
||||
|
|
@ -998,6 +1034,27 @@ class TestOutputPathSanitization:
|
|||
])
|
||||
assert result.exit_code != 0
|
||||
|
||||
def test_absolute_output_path_blocked(self):
|
||||
"""Absolute output path outside cwd should be rejected."""
|
||||
from typer.testing import CliRunner
|
||||
|
||||
from soup_cli.cli import app
|
||||
|
||||
runner = CliRunner()
|
||||
|
||||
with mock_patch(
|
||||
"soup_cli.commands.generate._generate_batch",
|
||||
return_value=[{"instruction": "x", "output": "y"}],
|
||||
):
|
||||
result = runner.invoke(app, [
|
||||
"data", "generate",
|
||||
"--prompt", "test",
|
||||
"--output", "/tmp/exfil.jsonl",
|
||||
"--count", "1",
|
||||
"--provider", "server",
|
||||
])
|
||||
assert result.exit_code != 0
|
||||
|
||||
|
||||
# ─── Ollama Model Shorthand Tests ───────────────────────────────────────
|
||||
|
||||
|
|
@ -1089,6 +1146,9 @@ class TestEndToEndGeneration:
|
|||
{"instruction": "What is AI?", "input": "", "output": "AI is..."},
|
||||
{"instruction": "Explain ML", "input": "", "output": "ML is..."},
|
||||
],
|
||||
), mock_patch(
|
||||
"soup_cli.commands.generate._path_within_cwd",
|
||||
return_value=True,
|
||||
):
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(app, [
|
||||
|
|
@ -1120,6 +1180,9 @@ class TestEndToEndGeneration:
|
|||
return_value=[
|
||||
{"instruction": "Write a function", "input": "", "output": "def foo(): pass"},
|
||||
],
|
||||
), mock_patch(
|
||||
"soup_cli.commands.generate._path_within_cwd",
|
||||
return_value=True,
|
||||
):
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(app, [
|
||||
|
|
@ -1149,6 +1212,9 @@ class TestEndToEndGeneration:
|
|||
return_value=[
|
||||
{"instruction": "What is AI?", "input": "", "output": "AI is..."},
|
||||
],
|
||||
), mock_patch(
|
||||
"soup_cli.commands.generate._path_within_cwd",
|
||||
return_value=True,
|
||||
):
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(app, [
|
||||
|
|
@ -1177,6 +1243,9 @@ class TestEndToEndGeneration:
|
|||
return_value=[
|
||||
{"instruction": "What is AI?", "input": "", "output": "AI is..."},
|
||||
],
|
||||
), mock_patch(
|
||||
"soup_cli.commands.generate._path_within_cwd",
|
||||
return_value=True,
|
||||
):
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(app, [
|
||||
|
|
|
|||
Loading…
Reference in New Issue