diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f2279d..bfc1714 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ All notable changes to weft are documented here. Format follows [Keep a Changelo - `Makefile` with `install`, `test`, `lint`, `clean` targets for one-command contributor onboarding. ### Fixed +- **Template name path traversal.** `templates.load_template`/`save_template` interpolated the caller-supplied `name` straight into `/.json`, so a name like `../../evil` escaped the templates directory (read or write). Names are now validated to a single safe stem (no separators, no `..`, no leading dot); legitimate names are unchanged, and the `weft start`/`preview` CLI paths reject a bad name cleanly. +- **Dashboard dependencies declared as the `dashboard` extra.** `core/dashboard.py` imports `textual` and `rich`, but `pyproject.toml` declared no runtime deps, so `weft dashboard` (and `import core.dashboard`) hard-failed with `ModuleNotFoundError` on a clean install. The two libraries now ship as `pip install weft[dashboard]` (keeping the core engine stdlib-only); the module imports regardless, and launching the dashboard without the extra now exits with an actionable `pip install weft[dashboard]` message instead of a traceback. - **`insights` step field is now actually populated.** `generate_context_md` rendered per-step `insights` (💡 lines) and the field was documented, but `start_workflow`/`rebuild_from_events` never copied it from the template into step state — so it silently rendered nothing. Both build sites now carry `insights`, matching the existing `suggest`-style optional fields. (`suggest` has the same latent gap; wire it the same way if a template ever uses it.) ### Changed diff --git a/core/cli.py b/core/cli.py index aca6c39..0ee070c 100755 --- a/core/cli.py +++ b/core/cli.py @@ -113,7 +113,11 @@ def cmd_start(args: list[str]) -> None: step_names = [s.strip() for s in name.split(",")] tmpl = templates.template_from_steps(step_names) else: - tmpl = templates.load_template(name, _project_dir()) + try: + tmpl = templates.load_template(name, _project_dir()) + except ValueError as e: + print(str(e), file=sys.stderr) + sys.exit(1) if not tmpl: print(f"Template not found: {name}", file=sys.stderr) print("Available:", file=sys.stderr) @@ -358,7 +362,11 @@ def cmd_preview(args: list[str]) -> None: print("Use 'start' with no args to list templates.", file=sys.stderr) sys.exit(1) - detail = templates.template_detail(args[0], _project_dir()) + try: + detail = templates.template_detail(args[0], _project_dir()) + except ValueError as e: + print(str(e), file=sys.stderr) + sys.exit(1) if not detail: print(f"Template not found: {args[0]}", file=sys.stderr) sys.exit(1) diff --git a/core/dashboard.py b/core/dashboard.py index ec97d43..95ceef2 100644 --- a/core/dashboard.py +++ b/core/dashboard.py @@ -7,13 +7,59 @@ import sys from pathlib import Path -from textual.app import App, ComposeResult -from textual.binding import Binding -from textual.containers import Horizontal, Vertical, VerticalScroll -from textual.widgets import Footer, Header, Static -from rich.panel import Panel -from rich.table import Table -from rich.text import Text +from typing import TYPE_CHECKING + +# textual + rich are optional — they ship only with the `dashboard` extra +# (`pip install weft[dashboard]`), keeping the core engine stdlib-only. Guard the +# import so this module still loads on a clean install; `_require_deps` surfaces a +# helpful message if someone actually launches the dashboard without them. +if TYPE_CHECKING: + # Type checkers always see the real symbols (deps present in a lint env). + from textual.app import App, ComposeResult + from textual.binding import Binding + from textual.containers import Horizontal, Vertical, VerticalScroll + from textual.widgets import Footer, Header, Static + from rich.panel import Panel + from rich.table import Table + from rich.text import Text + + _MISSING_DEP: str | None = None +else: + try: + from textual.app import App, ComposeResult + from textual.binding import Binding + from textual.containers import Horizontal, Vertical, VerticalScroll + from textual.widgets import Footer, Header, Static + from rich.panel import Panel + from rich.table import Table + from rich.text import Text + + _MISSING_DEP = None + except ModuleNotFoundError as exc: + _MISSING_DEP = exc.name + + # Minimal runtime fallbacks so the module imports and class/function + # definitions evaluate. None of these are used unless the dashboard is + # actually run, which `_require_deps` blocks first. + App = object + ComposeResult = object + + def Binding(*_args, **_kwargs): + return None + + Horizontal = Vertical = VerticalScroll = None + Footer = Header = Static = None + Panel = Table = Text = None + + +def _require_deps() -> None: + """Fail with an actionable message if the dashboard extra isn't installed.""" + if _MISSING_DEP is not None: + raise SystemExit( + f"weft dashboard requires the '{_MISSING_DEP}' package, which is not " + "installed.\nInstall the dashboard extra: pip install weft[dashboard]" + ) + sys.path.insert(0, str(Path(__file__).parent.parent)) @@ -461,6 +507,7 @@ class WeftDashboard(App): ] def __init__(self, project_dir: str | None = None): + _require_deps() super().__init__() self.project_dir = project_dir diff --git a/core/templates.py b/core/templates.py index d1e4681..57229ed 100644 --- a/core/templates.py +++ b/core/templates.py @@ -26,6 +26,19 @@ def _user_templates_dir() -> Path: return Path.home() / ".weft" / "templates" +def _safe_name(name: str) -> str: + """Validate a template name used to build a `/.json` path. + + A name must be a single safe path stem: no directory separators, no `..`, + and no leading dot. This blocks traversal like `../../evil` escaping the + templates directory. Legitimate names (e.g. `generic`, `feature-workflow`) + are returned unchanged. + """ + if not name or Path(name).name != name or name.startswith("."): + raise ValueError(f"Invalid template name: {name!r}") + return name + + def list_templates(project_dir: str | None = None) -> list[dict]: """List all available templates (plugin + user-global + project-local).""" templates = [] @@ -48,6 +61,7 @@ def list_templates(project_dir: str | None = None) -> list[dict]: def load_template(name: str, project_dir: str | None = None) -> dict | None: """Load a template by name. Project-local > user-global > plugin.""" + name = _safe_name(name) for d in [_project_templates_dir(project_dir), _user_templates_dir(), _plugin_templates_dir()]: path = d / f"{name}.json" if path.exists(): @@ -72,7 +86,7 @@ def template_from_steps(step_names: list[str], name: str = "adhoc") -> dict: def save_template(template: dict, project_dir: str | None = None) -> Path: """Save a template to the project-local templates directory. Returns the path.""" - name = template.get("name", "unnamed") + name = _safe_name(template.get("name", "unnamed")) d = _project_templates_dir(project_dir) d.mkdir(parents=True, exist_ok=True) path = d / f"{name}.json" diff --git a/pyproject.toml b/pyproject.toml index 684c1c3..42f6759 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,6 +29,10 @@ dependencies = [] [project.optional-dependencies] dev = ["pytest>=7"] +# The TUI dashboard (`weft dashboard`) is the one feature that needs third-party +# libraries. Kept out of the core install so the engine stays stdlib-only; +# install with `pip install weft[dashboard]`. +dashboard = ["textual>=0.40", "rich>=13.0"] [project.urls] Homepage = "https://github.com/dioptx/weft" diff --git a/tests/test_cli.py b/tests/test_cli.py index f45c0c5..c32dbaf 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -756,3 +756,20 @@ def test_unknown_command_points_at_help(self, project_dir): r = run_cli(["bogus"], str(project_dir)) assert r.returncode == 1 assert "--help" in r.stderr + + +class TestTemplateNameTraversalCLI: + """integration: a traversal template name is rejected cleanly end-to-end + (clean exit + message, not an unhandled traceback).""" + + def test_start_rejects_traversal(self, project_dir): + r = run_cli(["start", "../../evil"], str(project_dir)) + assert r.returncode == 1 + assert "Invalid template name" in r.stderr + assert "Traceback" not in r.stderr + + def test_preview_rejects_traversal(self, project_dir): + r = run_cli(["preview", "../../evil"], str(project_dir)) + assert r.returncode == 1 + assert "Invalid template name" in r.stderr + assert "Traceback" not in r.stderr diff --git a/tests/test_dashboard_deps.py b/tests/test_dashboard_deps.py new file mode 100644 index 0000000..7b27407 --- /dev/null +++ b/tests/test_dashboard_deps.py @@ -0,0 +1,103 @@ +"""Tests for the dashboard's optional-dependency handling. + +The TUI dashboard (`core/dashboard.py`) needs `textual` and `rich`. The core +engine is stdlib-only, so those ship as the `dashboard` extra +(`pip install weft[dashboard]`). These tests pin that contract: the extra is +declared, importing the module never hard-fails on a clean install, and +launching without the deps gives an actionable error. +""" + +import sys +from pathlib import Path + +import pytest + +PLUGIN_ROOT = Path(__file__).parent.parent + +# tomllib is stdlib on 3.11+; the repo supports 3.10+, so fall back to a text +# parse when it's unavailable. (dev extra has no tomli to lean on.) +try: + import tomllib +except ModuleNotFoundError: # pragma: no cover - only on Python 3.10 + tomllib = None + + +def _pyproject_text() -> str: + return (PLUGIN_ROOT / "pyproject.toml").read_text() + + +# --- frame / packaging: the extra is declared with both libraries --- +class TestDashboardExtraDeclared: + def _optional_deps(self) -> dict | None: + if tomllib is None: + return None + data = tomllib.loads(_pyproject_text()) + return data["project"].get("optional-dependencies", {}) + + def test_dashboard_extra_exists(self): + deps = self._optional_deps() + if deps is None: + assert "dashboard = [" in _pyproject_text() + else: + assert "dashboard" in deps, ( + "pyproject.toml must declare a [dashboard] extra so the " + "dashboard's third-party deps install via pip install weft[dashboard]" + ) + + def test_dashboard_extra_declares_textual_and_rich(self): + deps = self._optional_deps() + if deps is None: + blob = _pyproject_text().lower() + assert "dashboard = [" in blob + else: + blob = " ".join(deps.get("dashboard", [])).lower() + assert "textual" in blob + assert "rich" in blob + + def test_core_dependencies_stay_empty(self): + # The engine must remain stdlib-only; third-party deps belong in extras. + deps = self._optional_deps() + if deps is None: + assert "dependencies = []" in _pyproject_text() + else: + data = tomllib.loads(_pyproject_text()) + assert data["project"]["dependencies"] == [] + + +# --- frame: the module imports even when textual/rich are absent (regression) --- +class TestDashboardImportIsSoft: + def test_module_imports_without_hard_failure(self): + # Previously `import core.dashboard` raised ModuleNotFoundError on a clean + # install (textual/rich at module top level). It must now import and merely + # record the missing dep. + sys.modules.pop("core.dashboard", None) + import core.dashboard as dash + + assert hasattr(dash, "WeftDashboard") + assert hasattr(dash, "_MISSING_DEP") + + +# --- functional: launching without deps fails with a helpful message --- +class TestImportGuard: + def test_require_deps_raises_helpful_message(self, monkeypatch): + import core.dashboard as dash + + # Simulate the deps being absent regardless of what's installed locally. + monkeypatch.setattr(dash, "_MISSING_DEP", "textual") + with pytest.raises(SystemExit) as exc: + dash._require_deps() + assert "pip install weft[dashboard]" in str(exc.value) + + def test_require_deps_noop_when_present(self, monkeypatch): + import core.dashboard as dash + + monkeypatch.setattr(dash, "_MISSING_DEP", None) + dash._require_deps() # must not raise + + def test_constructing_dashboard_without_deps_is_guarded(self, monkeypatch): + import core.dashboard as dash + + monkeypatch.setattr(dash, "_MISSING_DEP", "textual") + with pytest.raises(SystemExit) as exc: + dash.WeftDashboard() + assert "pip install weft[dashboard]" in str(exc.value) diff --git a/tests/test_templates.py b/tests/test_templates.py index b75e2fd..ee16899 100644 --- a/tests/test_templates.py +++ b/tests/test_templates.py @@ -2,6 +2,8 @@ import json +import pytest + from core import templates @@ -178,3 +180,55 @@ def test_plan_has_push_guard(self, project_dir): plan = next(s for s in tmpl["steps"] if s["name"] == "plan-and-worktree") guards = plan.get("guards", []) assert len(guards) > 0 + + +class TestTemplateNameSafety: + """Template names are interpolated into `/.json`. A name must be a + single safe stem so it cannot escape the templates directory (path traversal). + """ + + # --- unit: the validator itself --- + def test_safe_name_passes_legitimate_stems(self): + for name in ["generic", "feature-workflow", "my_custom", "codex-review"]: + assert templates._safe_name(name) == name + + @pytest.mark.parametrize( + "name", + ["../../evil", "..", "a/b", "/etc/passwd", "sub/dir/x", ".hidden", "", "."], + ) + def test_safe_name_rejects_unsafe(self, name): + with pytest.raises(ValueError): + templates._safe_name(name) + + # --- security: load_template must not read outside the templates dirs --- + def test_load_template_rejects_traversal(self, project_dir, tmp_path): + # Plant a file one level above where a project-local "evil" template could + # land, then try to reach it via traversal. + secret = tmp_path / ".claude" / "weft" / "secret.json" + secret.write_text(json.dumps({"name": "secret", "steps": []})) + with pytest.raises(ValueError): + templates.load_template("../secret", str(project_dir)) + + def test_load_template_rejects_absolute(self, project_dir): + with pytest.raises(ValueError): + templates.load_template("/etc/passwd", str(project_dir)) + + # --- security: save_template must not write outside the project templates dir --- + def test_save_template_rejects_traversal(self, project_dir): + evil = {"name": "../../evil", "description": "x", "steps": [{"name": "s"}]} + with pytest.raises(ValueError): + templates.save_template(evil, str(project_dir)) + # Nothing escaped the sandbox. + assert not (project_dir.parent.parent / "evil.json").exists() + + # --- functional: legitimate names behave exactly as before (regression) --- + def test_legitimate_names_unaffected(self, project_dir): + assert templates.load_template("generic", str(project_dir)) is not None + assert templates.load_template("nonexistent", str(project_dir)) is None + + def test_save_then_load_roundtrip(self, project_dir): + tmpl = {"name": "my-flow", "description": "d", "steps": [{"name": "s"}]} + path = templates.save_template(tmpl, str(project_dir)) + assert path.name == "my-flow.json" + assert path.parent == project_dir / ".claude" / "weft" / "templates" + assert templates.load_template("my-flow", str(project_dir))["name"] == "my-flow"