From 15b8f034f5774f54dfcb6c7fc242650cb1179b76 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 01:13:52 -0700 Subject: [PATCH 1/9] fix(cli): bind loopback, keep secrets off argv, escape markup, add tasks show (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. `cf serve --host` and the server both defaulted to 0.0.0.0 while the printed hints said localhost — so a beta server exposing SQLite state, workspace file access and agent-execution endpoints sat on the LAN by default. Both now default to 127.0.0.1; .env.example no longer suggests 0.0.0.0. Binding a wildcard address warns, and warns much harder when CODEFRAME_AUTH_REQUIRED is disabled — that combination is an unauthenticated remote shell, and the dev docs suggest the flag. 2. `auth setup --value/-v` put the credential in argv, readable by any local process via /proc//cmdline and written to shell history — and the docstring taught it with real-looking keys. The option is now hidden and warns when used; --value-stdin and --value-file are the supported non-interactive paths, and the docstring examples show those. 3. Task titles and blocker text went into Rich-rendered output unescaped, so a title containing '[/b]' raised MarkupError and crashed `cf tasks list` and the TUI blocker log. Escaped at every user-controlled interpolation in cli/app.py and tui/app.py. Tests render five hostile-but-legitimate titles through the real CliRunner, plus one that proves the titles are hostile so the suite cannot pass vacuously. 4. `cf tasks show ` is advertised in README.md and CLAUDE.md and did not exist. Implemented: details, dependencies (with resolved titles), accepts a unique ID prefix, exits non-zero with a helpful message on an ambiguous or unknown id. 29 tests. --- .env.example | 2 +- codeframe/cli/app.py | 161 +++++++++++++++++--- codeframe/cli/auth_commands.py | 53 ++++++- codeframe/tui/app.py | 6 +- codeframe/ui/server.py | 6 +- tests/cli/test_cli_hardening_935.py | 222 ++++++++++++++++++++++++++++ 6 files changed, 421 insertions(+), 29 deletions(-) create mode 100644 tests/cli/test_cli_hardening_935.py diff --git a/.env.example b/.env.example index 3a79607a..cf8d2fd5 100644 --- a/.env.example +++ b/.env.example @@ -43,7 +43,7 @@ OPENAI_API_KEY=sk-... # Host to bind the Status Server to # Default: 0.0.0.0 (all interfaces) -# API_HOST=0.0.0.0 +# API_HOST=127.0.0.1 # loopback by default (#935); 0.0.0.0 exposes the API on your network # Port for the Status Server # Default: 8080 diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index d03aa6a1..85f4b3a7 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -23,6 +23,7 @@ import click import typer from rich.console import Console +from rich.markup import escape # Import auth subapp for credential management from codeframe.cli.auth_commands import auth_app @@ -707,10 +708,46 @@ def review( raise typer.Exit(1) +#: Bind addresses that accept connections from outside this machine. +_NON_LOOPBACK_BINDS = {"0.0.0.0", "::", "*", ""} + + +def _warn_if_exposed(host: str) -> None: + """Warn when binding beyond loopback, loudly if auth is off (#935). + + The server exposes SQLite state, workspace file access and + agent-execution endpoints. Exposing that on a LAN is a deliberate choice; + doing it with CODEFRAME_AUTH_REQUIRED=false — which the dev docs suggest — + is an unauthenticated remote shell, so that combination gets a stronger + warning rather than a quiet default. + """ + if host.strip() not in _NON_LOOPBACK_BINDS: + return + + auth_off = os.environ.get("CODEFRAME_AUTH_REQUIRED", "").strip().lower() in { + "0", "false", "no", "off", + } + console.print( + f"[yellow]WARNING:[/yellow] binding {escape(host)} accepts connections " + "from other machines on your network." + ) + if auth_off: + console.print( + "[bold red]CODEFRAME_AUTH_REQUIRED is disabled.[/bold red] The API — " + "including workspace file access and agent execution — is reachable " + "with no credentials. Do not use this combination outside a trusted " + "network." + ) + + @app.command() def serve( port: int = typer.Option(8080, "--port", "-p", help="Port to run server on"), - host: str = typer.Option("0.0.0.0", "--host", help="Host to bind to"), + host: str = typer.Option( + "127.0.0.1", + "--host", + help="Host to bind to. Use 0.0.0.0 to expose on the network (see warning).", + ), reload: bool = typer.Option(False, "--reload", help="Enable auto-reload (development)"), ) -> None: """Start the optional FastAPI server (wraps core). @@ -718,13 +755,19 @@ def serve( The server is NOT required for Golden Path commands. It provides the v2 REST API that wraps the same core functions. + Binds 127.0.0.1 by default (#935). This process exposes SQLite state, + workspace file access and agent-execution endpoints, so it is loopback-only + until you explicitly ask for otherwise. + Examples: cf serve cf serve --port 3000 - cf serve --reload + cf serve --host 0.0.0.0 # expose on the network — see the warning """ import uvicorn + _warn_if_exposed(host) + console.print(f"Starting CodeFRAME API server on {host}:{port}") console.print(f" Swagger UI: http://localhost:{port}/docs") console.print(f" ReDoc: http://localhost:{port}/redoc") @@ -1999,7 +2042,7 @@ def tasks_generate( console.print(f"\n[green]Generated {len(created)} tasks[/green]\n") for i, task in enumerate(created, 1): - console.print(f" {i}. {task.title}") + console.print(f" {i}. {escape(task.title)}") if task.description: # Show first line of description desc_preview = task.description.split("\n")[0][:60] @@ -2117,7 +2160,7 @@ def tasks_list( f"[{status_style}]{task.status.value}[/{status_style}]", str(task.priority), deps_str, - task.title[:55] + ("..." if len(task.title) > 55 else ""), + escape(task.title[:55]) + ("..." if len(task.title) > 55 else ""), ) console.print(table) @@ -2138,6 +2181,84 @@ def tasks_list( raise typer.Exit(1) +@tasks_app.command("show") +def tasks_show( + task_id: str = typer.Argument(..., help="Task ID (may be a unique prefix)"), + repo_path: Optional[Path] = typer.Option( + None, + "--workspace", "-w", + help="Workspace path (defaults to current directory)", + ), +) -> None: + """Show a task's details and dependencies. + + README.md and CLAUDE.md have advertised this command for some time while it + did not exist (#935). + + Examples: + cf tasks show 3f2a1b + cf tasks show 3f2a1b8c-... --workspace ../other-repo + """ + from codeframe.core.workspace import get_workspace + from codeframe.core import tasks + + workspace_path = repo_path or Path.cwd() + + try: + workspace = get_workspace(workspace_path) + except FileNotFoundError: + console.print(f"[red]Error:[/red] No workspace found at {workspace_path}") + console.print("Run 'codeframe init' to initialize a workspace first.") + raise typer.Exit(1) + + task = tasks.get(workspace, task_id) + if task is None: + matches = tasks.find_by_prefix(workspace, task_id) + if not matches: + console.print(f"[red]Error:[/red] No task matching '{escape(task_id)}'") + raise typer.Exit(1) + if len(matches) > 1: + console.print( + f"[red]Error:[/red] '{escape(task_id)}' matches {len(matches)} tasks:" + ) + for match in matches[:10]: + console.print(f" {match.id[:8]} {escape(match.title[:60])}") + raise typer.Exit(1) + task = matches[0] + + # escape(): a title or description may contain Rich markup like '[/b]', + # which would otherwise raise MarkupError instead of printing (#935). + console.print(f"\n[bold]{escape(task.title)}[/bold]") + console.print(f"[dim]{task.id}[/dim]\n") + console.print(f" Status: {task.status.value}") + console.print(f" Priority: {task.priority}") + if task.estimated_hours is not None: + console.print(f" Estimated: {task.estimated_hours}h") + if task.complexity_score is not None: + console.print(f" Complexity: {task.complexity_score}") + console.print(f" Created: {task.created_at:%Y-%m-%d %H:%M}") + console.print(f" Updated: {task.updated_at:%Y-%m-%d %H:%M}") + + if task.github_issue_number: + console.print(f" GitHub: #{task.github_issue_number} {escape(task.external_url or '')}") + + deps = task.depends_on or [] + console.print(f"\n[bold]Dependencies[/bold] ({len(deps)})") + if not deps: + console.print(" [dim]none[/dim]") + else: + titles = tasks.get_titles(workspace, deps) + for dep_id in deps: + title = titles.get(dep_id) + label = escape(title) if title else "[dim](missing)[/dim]" + console.print(f" {dep_id[:8]} {label}") + + if task.description: + console.print("\n[bold]Description[/bold]") + console.print(escape(task.description)) + console.print() + + @tasks_app.command("set") def tasks_set( attribute: str = typer.Argument(..., help="Attribute to set (e.g., 'status')"), @@ -2277,7 +2398,7 @@ def tasks_set( task = matching[0] if updated_count: console.print("[green]Task updated[/green]") - console.print(f" {task.title[:50]}") + console.print(f" {escape(task.title[:50])}") console.print(f" Status: {task.status.value} -> {new_status.value}") else: console.print(f"[yellow]Task already {new_status.value}[/yellow]") @@ -2379,7 +2500,7 @@ def tasks_delete( task = matching[0] if not force: - confirm = typer.confirm(f"Delete task '{task.title[:50]}'?") + confirm = typer.confirm(f"Delete task '{task.title[:50]}'?") # typer.confirm is not Rich-rendered if not confirm: console.print("[dim]Cancelled[/dim]") raise typer.Exit(0) @@ -2397,7 +2518,7 @@ def tasks_delete( deleted = tasks.delete(workspace, task.id) if deleted: - console.print(f"[green]Deleted task:[/green] {task.title[:50]}") + console.print(f"[green]Deleted task:[/green] {escape(task.title[:50])}") else: console.print("[red]Error:[/red] Failed to delete task") raise typer.Exit(1) @@ -2580,7 +2701,7 @@ def work_start( run = runtime.start_task_run(workspace, task.id) console.print("\n[bold green]Run started[/bold green]") - console.print(f" Task: {task.title}") + console.print(f" Task: {escape(task.title)}") console.print(f" Run ID: [dim]{run.id}[/dim]") console.print(" Status: [yellow]RUNNING[/yellow]") @@ -2608,7 +2729,7 @@ def work_start( elif state.status == AgentStatus.BLOCKED: console.print("[yellow]Task blocked - human input needed[/yellow]") if state.blocker: - console.print(f" Question: {state.blocker.question}") + console.print(f" Question: {escape(state.blocker.question)}") console.print(" Use 'codeframe blocker list' to see blockers") elif state.status == AgentStatus.FAILED: console.print("[red]Task execution failed[/red]") @@ -2703,7 +2824,7 @@ def work_resume( run = runtime.resume_run(workspace, task.id) console.print("\n[bold green]Run resumed[/bold green]") - console.print(f" Task: {task.title}") + console.print(f" Task: {escape(task.title)}") console.print(f" Run ID: [dim]{run.id}[/dim]") console.print(" Status: [yellow]RUNNING[/yellow]") @@ -2738,7 +2859,7 @@ def work_resume( elif state.status == AgentStatus.BLOCKED: console.print("[yellow]Task blocked - human input needed[/yellow]") if state.blocker: - console.print(f" Question: {state.blocker.question}") + console.print(f" Question: {escape(state.blocker.question)}") console.print(" Use 'codeframe blocker list' to see blockers") elif state.status == AgentStatus.FAILED: console.print("[red]Task execution failed[/red]") @@ -2808,7 +2929,7 @@ def work_stop( run = runtime.stop_run(workspace, task.id) console.print("\n[bold yellow]Run stopped[/bold yellow]") - console.print(f" Task: {task.title}") + console.print(f" Task: {escape(task.title)}") console.print(f" Run ID: [dim]{run.id}[/dim]") console.print(" Task returned to: [blue]READY[/blue]") @@ -3017,7 +3138,7 @@ def work_diagnose( failed_runs = [r for r in runs if r.status == runtime.RunStatus.FAILED] if not failed_runs: - console.print(f"[yellow]No failed run found for task '{task.title}'[/yellow]") + console.print(f"[yellow]No failed run found for task '{escape(task.title)}'[/yellow]") console.print("[dim]Diagnosis is only available for failed tasks.[/dim]") raise typer.Exit(1) @@ -3188,7 +3309,7 @@ def work_retry( raise typer.Exit(0) # Start new run - console.print(f"\n[bold]Retrying task:[/bold] {task.title}") + console.print(f"\n[bold]Retrying task:[/bold] {escape(task.title)}") run = runtime.start_task_run(workspace, task.id) console.print(f" Run ID: [dim]{run.id}[/dim]") @@ -3208,7 +3329,7 @@ def work_retry( elif state.status == AgentStatus.BLOCKED: console.print("[yellow]Task blocked - human input needed[/yellow]") if state.blocker: - console.print(f" Question: {state.blocker.question}") + console.print(f" Question: {escape(state.blocker.question)}") console.print(" Use 'codeframe blocker list' to see blockers") elif state.status == AgentStatus.FAILED: console.print("[red]Task execution failed[/red]") @@ -3274,7 +3395,7 @@ def work_update_description( tasks_module.update(workspace, task.id, description=description) console.print("[green]Task description updated[/green]") - console.print(f" Task: {task.title}") + console.print(f" Task: {escape(task.title)}") console.print(f" New description: {description[:100]}{'...' if len(description) > 100 else ''}") console.print("\nNext steps:") console.print(f" codeframe work retry {task.id[:8]} # Retry with updated description") @@ -3367,7 +3488,7 @@ def work_follow( console.print( f"[{status_color}]Run {last_run.status.value}[/{status_color}] " - f"for task: {task.title}" + f"for task: {escape(task.title)}" ) # Show final output if available @@ -3385,13 +3506,13 @@ def work_follow( raise typer.Exit(0) else: - console.print(f"[yellow]No active run found for task:[/yellow] {task.title}") + console.print(f"[yellow]No active run found for task:[/yellow] {escape(task.title)}") console.print("[dim]Start a run with:[/dim]") console.print(f" cf work start {task.id[:8]} --execute") raise typer.Exit(1) # We have an active run - stream it - console.print(f"[blue]Following task:[/blue] {task.title}") + console.print(f"[blue]Following task:[/blue] {escape(task.title)}") console.print(f"[dim]Run: {active_run.id[:8]} | Status: {active_run.status.value}[/dim]") # Show buffered output if requested @@ -5942,7 +6063,7 @@ def templates_apply( console.print(f"\n[green]Created {len(created_task_list)} tasks from template '{template_id}'[/green]\n") for i, task in enumerate(created_task_list, 1): - console.print(f" {i}. {task.title}") + console.print(f" {i}. {escape(task.title)}") console.print("\nNext steps:") console.print(" codeframe tasks list View all tasks") diff --git a/codeframe/cli/auth_commands.py b/codeframe/cli/auth_commands.py index bb4cd63c..d933ef77 100644 --- a/codeframe/cli/auth_commands.py +++ b/codeframe/cli/auth_commands.py @@ -35,6 +35,8 @@ import logging import os +import sys +from pathlib import Path from typing import Optional, Tuple import requests @@ -537,7 +539,24 @@ def setup_credential( None, "--provider", "-p", help="Provider name (e.g., anthropic, github, openai)" ), value: Optional[str] = typer.Option( - None, "--value", "-v", help="Credential value (API key or token)", hide_input=True + None, + "--value", + "-v", + help=( + "DEPRECATED — the value appears in /proc//cmdline and your " + "shell history. Pipe it on stdin or omit this to be prompted." + ), + hidden=True, + ), + value_stdin: bool = typer.Option( + False, + "--value-stdin", + help="Read the credential value from stdin (for scripts and CI).", + ), + value_file: Optional[Path] = typer.Option( + None, + "--value-file", + help="Read the credential value from a file (the first line).", ), ): """Configure a credential for a provider. @@ -546,16 +565,30 @@ def setup_credential( and other integrations. Credentials are stored in the system keyring or an encrypted file. + The value is never taken from the command line by default (#935): anything + in argv is world-readable via /proc//cmdline while the process runs and + is written to your shell history. + Examples: - codeframe auth setup # Interactive mode + codeframe auth setup # interactive — prompts, input hidden + + codeframe auth setup --provider anthropic # prompts for the value - codeframe auth setup --provider anthropic --value sk-ant-... + echo "$MY_KEY" | codeframe auth setup -p github --value-stdin - codeframe auth setup -p github -v ghp_... + codeframe auth setup -p github --value-file ~/.secrets/gh-token """ manager = CredentialManager() + if value is not None: + console.print( + "[yellow]WARNING:[/yellow] --value puts the credential in this " + "process's command line, readable by any local process via " + "/proc//cmdline, and in your shell history. Use --value-stdin " + "or --value-file instead." + ) + # Interactive provider selection if not provided if not provider: console.print("\n[bold]Select a provider to configure:[/bold]\n") @@ -581,6 +614,18 @@ def setup_credential( console.print(f"[red]Error:[/red] {e}") raise typer.Exit(1) + # stdin/file take precedence over the deprecated option. + if value_stdin: + # .readline() not .read(): a trailing newline from `echo` is not part of + # the secret, and a here-doc may carry more than one line. + value = sys.stdin.readline().strip() + elif value_file is not None: + try: + value = value_file.read_text(encoding="utf-8").splitlines()[0].strip() + except (OSError, IndexError, UnicodeDecodeError) as exc: + console.print(f"[red]Error:[/red] could not read {value_file}: {exc}") + raise typer.Exit(1) + # Prompt for value if not provided if not value: console.print(f"\nConfiguring [bold]{provider_enum.display_name}[/bold]") diff --git a/codeframe/tui/app.py b/codeframe/tui/app.py index e96db09b..114614ff 100644 --- a/codeframe/tui/app.py +++ b/codeframe/tui/app.py @@ -6,6 +6,8 @@ from typing import Optional +from rich.markup import escape + from textual.app import App, ComposeResult from textual.binding import Binding from textual.containers import Horizontal, Vertical @@ -219,7 +221,7 @@ def _update_task_table(self, data: DashboardData) -> None: color = _STATUS_COLORS.get(status_val, "white") table.add_row( task.id[:8], - task.title[:50], + escape(task.title[:50]), f"[{color}]{status_val}[/{color}]", str(task.priority), ) @@ -241,7 +243,7 @@ def _update_blocker_panel(self, data: DashboardData) -> None: return for blocker in data.blockers: - log.write(f"[red bold]{blocker.id[:8]}[/red bold]: {blocker.question[:60]}") + log.write(f"[red bold]{blocker.id[:8]}[/red bold]: {escape(blocker.question[:60])}") _SEVERITY_COLORS: dict[str, str] = { "critical": "red bold", diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index 13a507b7..6bba7ace 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -881,7 +881,7 @@ async def test_broadcast(message: dict, project_id: int = None): # ============================================================================ -def run_server(host: str = "0.0.0.0", port: int = 8080): +def run_server(host: str = "127.0.0.1", port: int = 8080): """Run the Status Server.""" import uvicorn @@ -896,7 +896,9 @@ def run_server(host: str = "0.0.0.0", port: int = 8080): parser.add_argument( "--host", type=str, - default=os.environ.get("HOST", "0.0.0.0"), + # Loopback by default (#935): this process exposes SQLite state, + # workspace file access and agent execution. Set HOST=0.0.0.0 to expose it. + default=os.environ.get("HOST", "127.0.0.1"), help="Host to bind to (default: 0.0.0.0 or HOST env var)", ) parser.add_argument( diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py new file mode 100644 index 00000000..bdae593e --- /dev/null +++ b/tests/cli/test_cli_hardening_935.py @@ -0,0 +1,222 @@ +"""CLI defaults and output hardening (#935). + +Four separate problems: + +1. `cf serve` and the server defaulted `--host` to 0.0.0.0 while the printed + hints said localhost — so a beta server exposing SQLite state, workspace file + access and agent-execution endpoints sat on the LAN by default. Combined with + the documented `CODEFRAME_AUTH_REQUIRED=false` dev mode that is an + unauthenticated remote shell. +2. `auth setup` accepted the credential via `--value/-v` and the docstring + taught it, exposing keys through /proc//cmdline and shell history. +3. Task titles and blocker text were interpolated into Rich-rendered output with + markup enabled, so a title containing '[/b]' raised MarkupError and crashed + `cf tasks list` and the TUI. +4. README.md and CLAUDE.md advertised `cf tasks show `, which did not exist. +""" + +import inspect +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from codeframe.cli.app import app, tasks_show +from codeframe.core import tasks +from codeframe.core.state_machine import TaskStatus +from codeframe.core.workspace import create_or_load_workspace + +pytestmark = pytest.mark.v2 + +REPO_ROOT = Path(__file__).resolve().parents[2] + +#: Titles that are valid user input and also valid-looking Rich markup. +HOSTILE_TITLES = [ + "Fix the [/b] parser", + "Support [bold] tags", + "Handle [/] gracefully", + "Array indexing a[0] and b[1]", + "[red]not actually markup[/red]", +] + + +@pytest.fixture +def workspace(tmp_path): + return create_or_load_workspace(tmp_path) + + +class TestServeBindsLoopback: + def test_serve_host_default_is_loopback(self): + from codeframe.cli.app import serve + + default = inspect.signature(serve).parameters["host"].default + assert default.default == "127.0.0.1", ( + f"cf serve binds {default.default} by default" + ) + + def test_run_server_default_is_loopback(self): + from codeframe.ui.server import run_server + + assert inspect.signature(run_server).parameters["host"].default == "127.0.0.1" + + def test_env_example_does_not_suggest_a_public_bind(self): + content = (REPO_ROOT / ".env.example").read_text() + + assert "# API_HOST=0.0.0.0" not in content + + def test_exposing_the_bind_warns(self, capsys): + from codeframe.cli.app import _warn_if_exposed + + _warn_if_exposed("0.0.0.0") + + assert "WARNING" in capsys.readouterr().out + + def test_loopback_does_not_warn(self, capsys): + from codeframe.cli.app import _warn_if_exposed + + _warn_if_exposed("127.0.0.1") + + assert capsys.readouterr().out == "" + + def test_exposed_bind_with_auth_disabled_warns_harder(self, capsys, monkeypatch): + """The dangerous combination the issue calls out.""" + from codeframe.cli.app import _warn_if_exposed + + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "false") + _warn_if_exposed("0.0.0.0") + + out = capsys.readouterr().out + assert "CODEFRAME_AUTH_REQUIRED" in out + assert "no credentials" in out + + @pytest.mark.parametrize("bind", ["0.0.0.0", "::", "*"]) + def test_every_wildcard_bind_warns(self, capsys, bind): + from codeframe.cli.app import _warn_if_exposed + + _warn_if_exposed(bind) + + assert "WARNING" in capsys.readouterr().out + + +class TestCredentialsStayOffArgv: + def test_stdin_and_file_options_exist(self): + from codeframe.cli.auth_commands import setup_credential + + params = inspect.signature(setup_credential).parameters + assert "value_stdin" in params + assert "value_file" in params + + def test_value_option_is_hidden_from_help(self): + from codeframe.cli.auth_commands import setup_credential + + value_param = inspect.signature(setup_credential).parameters["value"].default + assert value_param.hidden is True, "--value must not be advertised" + assert "DEPRECATED" in value_param.help + + def test_docstring_no_longer_teaches_passing_the_secret_inline(self): + from codeframe.cli.auth_commands import setup_credential + + doc = setup_credential.__doc__ or "" + assert "--value sk-ant-" not in doc + assert "-v ghp_" not in doc + assert "--value-stdin" in doc, "the safe alternative must be shown" + + +class TestRichMarkupIsEscaped: + """AC3 — a title containing '[/b]' must render, not raise.""" + + @pytest.mark.parametrize("title", HOSTILE_TITLES) + def test_tasks_list_renders_a_hostile_title(self, workspace, title, tmp_path): + tasks.create(workspace, title=title, description="d", status=TaskStatus.READY) + + result = CliRunner().invoke(app, ["tasks", "list", "--workspace", str(tmp_path)]) + + assert result.exception is None, f"{title!r} crashed: {result.exception!r}" + assert result.exit_code == 0 + + @pytest.mark.parametrize("title", HOSTILE_TITLES) + def test_tasks_show_renders_a_hostile_title(self, workspace, title, tmp_path): + task = tasks.create( + workspace, title=title, description=f"see {title}", status=TaskStatus.READY + ) + + result = CliRunner().invoke( + app, ["tasks", "show", task.id[:8], "--workspace", str(tmp_path)] + ) + + assert result.exception is None, f"{title!r} crashed: {result.exception!r}" + assert result.exit_code == 0 + + def test_markup_is_shown_literally_not_interpreted(self, workspace, tmp_path): + tasks.create( + workspace, title="Fix [/b] now", description="d", status=TaskStatus.READY + ) + + result = CliRunner().invoke(app, ["tasks", "list", "--workspace", str(tmp_path)]) + + assert "[/b]" in result.output, "the literal text should survive escaping" + + def test_rich_would_have_raised_without_escaping(self): + """Guard the guard: prove the hostile titles really are hostile.""" + from rich.console import Console + + console = Console(file=open("/dev/null", "w")) + with pytest.raises(Exception): + console.print(f"[cyan]Title:[/cyan] {HOSTILE_TITLES[0]}") + + +class TestTasksShowExists: + """AC4 — README.md and CLAUDE.md advertise it.""" + + def test_command_is_registered(self): + result = CliRunner().invoke(app, ["tasks", "--help"]) + + assert "show" in result.output + + def test_shows_details_and_dependencies(self, workspace, tmp_path): + dep = tasks.create(workspace, title="First", description="", status=TaskStatus.READY) + task = tasks.create( + workspace, + title="Second", + description="Do the thing", + status=TaskStatus.READY, + depends_on=[dep.id], + ) + + result = CliRunner().invoke( + app, ["tasks", "show", task.id, "--workspace", str(tmp_path)] + ) + + assert result.exit_code == 0, result.output + assert "Second" in result.output + assert "Do the thing" in result.output + assert "Dependencies" in result.output + assert dep.id[:8] in result.output + assert "First" in result.output, "the dependency's title should resolve" + + def test_accepts_a_unique_prefix(self, workspace, tmp_path): + task = tasks.create(workspace, title="Prefixed", description="", status=TaskStatus.READY) + + result = CliRunner().invoke( + app, ["tasks", "show", task.id[:8], "--workspace", str(tmp_path)] + ) + + assert result.exit_code == 0, result.output + assert "Prefixed" in result.output + + def test_unknown_id_exits_nonzero(self, workspace, tmp_path): + result = CliRunner().invoke( + app, ["tasks", "show", "nosuchtask", "--workspace", str(tmp_path)] + ) + + assert result.exit_code == 1 + assert "No task matching" in result.output + + def test_docs_reference_a_command_that_exists(self): + for doc in ("README.md", "CLAUDE.md"): + content = (REPO_ROOT / doc).read_text() + if "tasks show" in content: + assert tasks_show is not None + break + else: + pytest.fail("neither doc mentions `tasks show` — did the reference move?") From ef31097385626ec4bf1a83740feb3bc2119089c5 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 01:14:48 -0700 Subject: [PATCH 2/9] test(cli): import tasks_show inside its test, not at module scope (#935) A module-level import turned 'the command is missing' into a collection error against main, which hid the other 17 RED tests behind one ImportError. --- tests/cli/test_cli_hardening_935.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py index bdae593e..9e6d1a2f 100644 --- a/tests/cli/test_cli_hardening_935.py +++ b/tests/cli/test_cli_hardening_935.py @@ -21,7 +21,7 @@ import pytest from typer.testing import CliRunner -from codeframe.cli.app import app, tasks_show +from codeframe.cli.app import app from codeframe.core import tasks from codeframe.core.state_machine import TaskStatus from codeframe.core.workspace import create_or_load_workspace @@ -216,6 +216,11 @@ def test_docs_reference_a_command_that_exists(self): for doc in ("README.md", "CLAUDE.md"): content = (REPO_ROOT / doc).read_text() if "tasks show" in content: + # Imported here, not at module scope: a module-level import + # would turn "the command is missing" into a collection error + # that hides every other test in this file. + from codeframe.cli.app import tasks_show + assert tasks_show is not None break else: From c6e1162ef75c74927c52c82be1d30804b7cee27b Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 01:26:15 -0700 Subject: [PATCH 3/9] fix(cli): default GlobalConfig.api_host to loopback too (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught tests/cli/test_serve_command.py asserting the OLD 0.0.0.0 default — the suite was pinning the vulnerability in place. Updated with a comment saying why, the same way #929 handled tests that cemented a bug. That surfaced a third default the issue did not name: GlobalConfig.api_host (core/config.py) also defaulted to 0.0.0.0, so API_HOST-driven deployments kept binding every interface even after `cf serve` and run_server were fixed. tests/cli + tests/config: 557 passed. --- codeframe/core/config.py | 4 +- tests/cli/test_serve_command.py | 10 ++- tests/config/test_config.py | 4 +- web-ui/security-headers.js | 68 ++++++++++++++----- web-ui/src/__tests__/security-headers.test.js | 68 ++++++++++++++++++- web-ui/src/proxy.ts | 59 ++++++++++++++++ 6 files changed, 191 insertions(+), 22 deletions(-) create mode 100644 web-ui/src/proxy.ts diff --git a/codeframe/core/config.py b/codeframe/core/config.py index f2b2ace7..c8d9a61a 100644 --- a/codeframe/core/config.py +++ b/codeframe/core/config.py @@ -683,7 +683,9 @@ class GlobalConfig(BaseSettings): database_path: str = Field(".codeframe/state.db", alias="DATABASE_PATH") # Status Server configuration - api_host: str = Field("0.0.0.0", alias="API_HOST") + # Loopback by default (#935): the API exposes SQLite state, workspace + # file access and agent execution. Set API_HOST=0.0.0.0 to expose it. + api_host: str = Field("127.0.0.1", alias="API_HOST") api_port: int = Field(8080, alias="API_PORT") cors_origins: str = Field("http://localhost:3000,http://localhost:5173", alias="CORS_ORIGINS") diff --git a/tests/cli/test_serve_command.py b/tests/cli/test_serve_command.py index 1e17a4c7..5ad77375 100644 --- a/tests/cli/test_serve_command.py +++ b/tests/cli/test_serve_command.py @@ -27,7 +27,13 @@ class TestServeCommand: """`cf serve` wires uvicorn with the requested host/port/reload.""" def test_default_host_port_and_app(self): - """No flags → uvicorn.run on the v2 app at 0.0.0.0:8080, reload off.""" + """No flags → uvicorn.run on the v2 app at 127.0.0.1:8080, reload off. + + The default was 0.0.0.0 until #935 — this test asserted it, which meant + the suite was pinning the vulnerability in place: the server exposes + SQLite state, workspace file access and agent execution, and binding + every interface by default put all of it on the LAN. + """ with patch("uvicorn.run") as mock_run: result = runner.invoke(app, ["serve"]) @@ -36,7 +42,7 @@ def test_default_host_port_and_app(self): args, kwargs = mock_run.call_args # The app import string is passed positionally to uvicorn.run. assert args[0] == "codeframe.ui.server:app" - assert kwargs["host"] == "0.0.0.0" + assert kwargs["host"] == "127.0.0.1" assert kwargs["port"] == 8080 assert kwargs["reload"] is False diff --git a/tests/config/test_config.py b/tests/config/test_config.py index 57b35871..1f36aa17 100644 --- a/tests/config/test_config.py +++ b/tests/config/test_config.py @@ -22,7 +22,9 @@ def test_default_values(self, monkeypatch): config = GlobalConfig(_env_file=None) assert config.database_path == ".codeframe/state.db" - assert config.api_host == "0.0.0.0" + # 127.0.0.1 since #935 — binding every interface by default put + # workspace file access and agent execution on the LAN. + assert config.api_host == "127.0.0.1" assert config.api_port == 8080 assert config.log_level == "INFO" assert config.debug is False diff --git a/web-ui/security-headers.js b/web-ui/security-headers.js index c2e6b293..d93d7701 100644 --- a/web-ui/security-headers.js +++ b/web-ui/security-headers.js @@ -25,26 +25,54 @@ function buildConnectSrc({ apiUrl, wsUrl } = {}) { return Array.from(sources).join(' '); } -function buildCsp(env = process.env) { +/** + * Build the script-src directive. + * + * With a per-request nonce (the production path, set by proxy.ts) this is + * `'self' 'nonce-' 'strict-dynamic'` and contains NO 'unsafe-inline', so an + * injected inline script simply does not execute — which is what protects the + * localStorage JWT (#936). 'strict-dynamic' lets Next.js's nonced bootstrap + * load the chunks it needs without enumerating them. + * + * Browsers that understand 'strict-dynamic' ignore 'self' for scripts; it is + * kept for older ones, which then fall back to a host allow-list rather than + * to nothing. + * + * Without a nonce we cannot serve a working App Router page, so the nonce-less + * form keeps 'unsafe-inline'. That form must only ever reach responses that are + * not HTML documents — see next.config.js. + */ +function buildScriptSrc({ nonce, isDev } = {}) { + // unsafe-eval is only needed by the Next.js dev runtime (React Refresh / + // eval source maps); production bundles never eval, so it ships dev-only. + const devEval = isDev ? " 'unsafe-eval'" : ''; + if (nonce) { + return `script-src 'self' 'nonce-${nonce}' 'strict-dynamic'${devEval}`; + } + return `script-src 'self' 'unsafe-inline'${devEval}`; +} + +function buildCsp(env = process.env, { nonce } = {}) { const connectSrc = buildConnectSrc({ apiUrl: env.NEXT_PUBLIC_API_URL, wsUrl: env.NEXT_PUBLIC_WS_URL, }); - // unsafe-eval is only needed by the Next.js dev runtime (React Refresh / - // eval source maps); production bundles never eval, so it ships dev-only. - const scriptSrc = env.NODE_ENV === 'development' - ? "script-src 'self' 'unsafe-inline' 'unsafe-eval'" - : "script-src 'self' 'unsafe-inline'"; + const scriptSrc = buildScriptSrc({ + nonce, + isDev: env.NODE_ENV === 'development', + }); return [ "default-src 'self'", - // ponytail: 'unsafe-inline' is required by the Next.js App Router without - // a per-request nonce middleware (a much larger change). Residual risk - // (#783): with the JWT in localStorage and inline scripts allowed, an - // injected inline script can read the token. connect-src/img-src/ - // object-src below close the fetch/XHR/img/plugin exfil channels, but NOT - // top-level navigation (`window.location = attacker + token`) — CSP has - // no deployable navigate-to directive, so that channel stays open until - // the real fix: nonce middleware and/or an httpOnly-cookie token. + // script-src carries a per-request nonce in production (#936), so an + // injected inline script does not run and cannot read the localStorage + // JWT. That closes the top-level-navigation exfil channel too + // (`window.location = attacker + token`), which no CSP directive could + // block once the script was already executing — the residual risk this + // comment used to concede (#783). + // + // style-src deliberately keeps 'unsafe-inline': Tailwind and React set + // style attributes directly, and a stolen *style* is not a stolen session. + // The threat this addresses is script execution. scriptSrc, "style-src 'self' 'unsafe-inline'", `img-src 'self' data: blob: ${AVATAR_HOST}`, @@ -57,13 +85,21 @@ function buildCsp(env = process.env) { ].join('; '); } +/** + * Static headers for next.config.js. + * + * NOTE (#936): no Content-Security-Policy here. HTML documents get their CSP + * from proxy.ts, which mints a per-request nonce; a static CSP cannot carry one + * and would have to keep 'unsafe-inline'. Two CSP headers on one response are + * intersected by the browser, so leaving the static one in place would have + * re-imposed the weaker policy's absence of a nonce and blocked every script. + */ function securityHeaders(env = process.env) { return [ - { key: 'Content-Security-Policy', value: buildCsp(env) }, { key: 'X-Content-Type-Options', value: 'nosniff' }, { key: 'X-Frame-Options', value: 'DENY' }, { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, ]; } -module.exports = { buildCsp, buildConnectSrc, securityHeaders }; +module.exports = { buildCsp, buildConnectSrc, buildScriptSrc, securityHeaders }; diff --git a/web-ui/src/__tests__/security-headers.test.js b/web-ui/src/__tests__/security-headers.test.js index 8deb806f..d0501710 100644 --- a/web-ui/src/__tests__/security-headers.test.js +++ b/web-ui/src/__tests__/security-headers.test.js @@ -49,11 +49,14 @@ describe('security headers (#657)', () => { expect(buildCsp({ NODE_ENV: 'development' })).toContain("'unsafe-eval'"); }); - test('securityHeaders ships the CSP plus the hardening header set', () => { + test('securityHeaders ships the hardening header set', () => { + // The CSP moved OUT of this static set in #936: HTML documents get it from + // proxy.ts, which mints a per-request nonce. A static CSP cannot carry one, + // and two CSP headers on a response are intersected — so leaving it here + // would have blocked every script. Asserted explicitly below. const keys = securityHeaders({}).map((h) => h.key); expect(keys).toEqual( expect.arrayContaining([ - 'Content-Security-Policy', 'X-Content-Type-Options', 'X-Frame-Options', 'Referrer-Policy', @@ -61,3 +64,64 @@ describe('security headers (#657)', () => { ); }); }); + +describe('production CSP carries a nonce, not unsafe-inline (#936)', () => { + const PROD = { NODE_ENV: 'production' }; + + /** The AC's CI check: the production policy must not allow inline script. */ + test('script-src has no unsafe-inline in production', () => { + const scriptSrc = buildCsp(PROD, { nonce: 'test-nonce' }) + .split('; ') + .find((d) => d.startsWith('script-src')); + + expect(scriptSrc).toBeDefined(); + expect(scriptSrc).not.toContain("'unsafe-inline'"); + }); + + test('script-src carries the request nonce and strict-dynamic', () => { + const scriptSrc = buildCsp(PROD, { nonce: 'abc123' }) + .split('; ') + .find((d) => d.startsWith('script-src')); + + expect(scriptSrc).toContain("'nonce-abc123'"); + expect(scriptSrc).toContain("'strict-dynamic'"); + }); + + test('production never ships unsafe-eval', () => { + expect(buildCsp(PROD, { nonce: 'n' })).not.toContain("'unsafe-eval'"); + }); + + test('development keeps unsafe-eval for the Next.js dev runtime', () => { + const csp = buildCsp({ NODE_ENV: 'development' }, { nonce: 'n' }); + expect(csp).toContain("'unsafe-eval'"); + // ...but still no unsafe-inline: the nonce works in dev too. + const scriptSrc = csp.split('; ').find((d) => d.startsWith('script-src')); + expect(scriptSrc).not.toContain("'unsafe-inline'"); + }); + + test('style-src deliberately keeps unsafe-inline', () => { + // Documented decision: Tailwind/React set style attributes directly, and a + // stolen style is not a stolen session. If this ever changes it should be a + // conscious edit, not a silent one. + const styleSrc = buildCsp(PROD, { nonce: 'n' }) + .split('; ') + .find((d) => d.startsWith('style-src')); + expect(styleSrc).toContain("'unsafe-inline'"); + }); + + test('the static header set no longer carries a CSP', () => { + // Two CSP headers are intersected by the browser, so a static (nonce-less) + // one alongside the proxy's would block every script. + const keys = securityHeaders(PROD).map((h) => h.key); + expect(keys).not.toContain('Content-Security-Policy'); + expect(keys).toContain('X-Content-Type-Options'); + }); + + test('a missing nonce still produces a working policy', () => { + // Non-document responses fall back to this; it must not be empty or broken. + const scriptSrc = buildCsp(PROD) + .split('; ') + .find((d) => d.startsWith('script-src')); + expect(scriptSrc).toContain("'self'"); + }); +}); diff --git a/web-ui/src/proxy.ts b/web-ui/src/proxy.ts new file mode 100644 index 00000000..ebfc46a7 --- /dev/null +++ b/web-ui/src/proxy.ts @@ -0,0 +1,59 @@ +/** + * Per-request CSP nonce (#936). + * + * The production CSP used to ship `script-src 'self' 'unsafe-inline'`, and the + * security-headers comment conceded the consequence: with the JWT in + * localStorage and inline scripts allowed, any future HTML-injection bug became + * full session theft. connect-src/img-src closed fetch/XHR/img exfiltration, but + * nothing could close top-level navigation (`window.location = attacker + token`) + * once the injected script was already running. + * + * A nonce fixes the actual problem rather than the exfil channels: an injected + * inline script has no nonce, so it never executes. + * + * This must be a proxy (not a static `headers()` entry) because the nonce has to + * differ per request — a constant nonce is worth exactly as much as + * 'unsafe-inline'. + */ +import { NextResponse } from 'next/server'; +import type { NextRequest } from 'next/server'; + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { buildCsp } = require('../security-headers'); + +export function proxy(request: NextRequest) { + // crypto.getRandomValues: available on the Edge runtime, unlike node:crypto. + const bytes = new Uint8Array(16); + crypto.getRandomValues(bytes); + const nonce = btoa(String.fromCharCode(...bytes)); + + const csp = buildCsp(process.env, { nonce }); + + // Set on the REQUEST headers too: Next.js reads the nonce from the CSP header + // it receives and stamps it onto the framework's own inline bootstrap scripts. + // Without this the page's own scripts would be blocked by the policy we just + // set — the failure mode is a blank page, so it is not subtle. + const requestHeaders = new Headers(request.headers); + requestHeaders.set('x-nonce', nonce); + requestHeaders.set('Content-Security-Policy', csp); + + const response = NextResponse.next({ request: { headers: requestHeaders } }); + response.headers.set('Content-Security-Policy', csp); + return response; +} + +export const config = { + matcher: [ + { + // Everything except API routes, static assets and the favicon. Those are + // not HTML documents, so they need no nonce; next.config.js still applies + // the non-CSP hardening headers to them. + source: '/((?!api|_next/static|_next/image|favicon.ico).*)', + // Prefetches are not rendered, so nonce-ing them only costs work. + missing: [ + { type: 'header', key: 'next-router-prefetch' }, + { type: 'header', key: 'purpose', value: 'prefetch' }, + ], + }, + ], +}; From 8f578ce1c3894b082831d39e26656b410eeaf54c Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 01:27:22 -0700 Subject: [PATCH 4/9] chore: move the CSP nonce work out of this PR (#935) security-headers.js, its test and proxy.ts belong to #936 (CSP nonce) and were swept in here by a `git add -A`. They are unrelated to the CLI hardening this PR is for; re-applied on the #936 branch. --- web-ui/security-headers.js | 68 +++++-------------- web-ui/src/__tests__/security-headers.test.js | 68 +------------------ web-ui/src/proxy.ts | 59 ---------------- 3 files changed, 18 insertions(+), 177 deletions(-) delete mode 100644 web-ui/src/proxy.ts diff --git a/web-ui/security-headers.js b/web-ui/security-headers.js index d93d7701..c2e6b293 100644 --- a/web-ui/security-headers.js +++ b/web-ui/security-headers.js @@ -25,54 +25,26 @@ function buildConnectSrc({ apiUrl, wsUrl } = {}) { return Array.from(sources).join(' '); } -/** - * Build the script-src directive. - * - * With a per-request nonce (the production path, set by proxy.ts) this is - * `'self' 'nonce-' 'strict-dynamic'` and contains NO 'unsafe-inline', so an - * injected inline script simply does not execute — which is what protects the - * localStorage JWT (#936). 'strict-dynamic' lets Next.js's nonced bootstrap - * load the chunks it needs without enumerating them. - * - * Browsers that understand 'strict-dynamic' ignore 'self' for scripts; it is - * kept for older ones, which then fall back to a host allow-list rather than - * to nothing. - * - * Without a nonce we cannot serve a working App Router page, so the nonce-less - * form keeps 'unsafe-inline'. That form must only ever reach responses that are - * not HTML documents — see next.config.js. - */ -function buildScriptSrc({ nonce, isDev } = {}) { - // unsafe-eval is only needed by the Next.js dev runtime (React Refresh / - // eval source maps); production bundles never eval, so it ships dev-only. - const devEval = isDev ? " 'unsafe-eval'" : ''; - if (nonce) { - return `script-src 'self' 'nonce-${nonce}' 'strict-dynamic'${devEval}`; - } - return `script-src 'self' 'unsafe-inline'${devEval}`; -} - -function buildCsp(env = process.env, { nonce } = {}) { +function buildCsp(env = process.env) { const connectSrc = buildConnectSrc({ apiUrl: env.NEXT_PUBLIC_API_URL, wsUrl: env.NEXT_PUBLIC_WS_URL, }); - const scriptSrc = buildScriptSrc({ - nonce, - isDev: env.NODE_ENV === 'development', - }); + // unsafe-eval is only needed by the Next.js dev runtime (React Refresh / + // eval source maps); production bundles never eval, so it ships dev-only. + const scriptSrc = env.NODE_ENV === 'development' + ? "script-src 'self' 'unsafe-inline' 'unsafe-eval'" + : "script-src 'self' 'unsafe-inline'"; return [ "default-src 'self'", - // script-src carries a per-request nonce in production (#936), so an - // injected inline script does not run and cannot read the localStorage - // JWT. That closes the top-level-navigation exfil channel too - // (`window.location = attacker + token`), which no CSP directive could - // block once the script was already executing — the residual risk this - // comment used to concede (#783). - // - // style-src deliberately keeps 'unsafe-inline': Tailwind and React set - // style attributes directly, and a stolen *style* is not a stolen session. - // The threat this addresses is script execution. + // ponytail: 'unsafe-inline' is required by the Next.js App Router without + // a per-request nonce middleware (a much larger change). Residual risk + // (#783): with the JWT in localStorage and inline scripts allowed, an + // injected inline script can read the token. connect-src/img-src/ + // object-src below close the fetch/XHR/img/plugin exfil channels, but NOT + // top-level navigation (`window.location = attacker + token`) — CSP has + // no deployable navigate-to directive, so that channel stays open until + // the real fix: nonce middleware and/or an httpOnly-cookie token. scriptSrc, "style-src 'self' 'unsafe-inline'", `img-src 'self' data: blob: ${AVATAR_HOST}`, @@ -85,21 +57,13 @@ function buildCsp(env = process.env, { nonce } = {}) { ].join('; '); } -/** - * Static headers for next.config.js. - * - * NOTE (#936): no Content-Security-Policy here. HTML documents get their CSP - * from proxy.ts, which mints a per-request nonce; a static CSP cannot carry one - * and would have to keep 'unsafe-inline'. Two CSP headers on one response are - * intersected by the browser, so leaving the static one in place would have - * re-imposed the weaker policy's absence of a nonce and blocked every script. - */ function securityHeaders(env = process.env) { return [ + { key: 'Content-Security-Policy', value: buildCsp(env) }, { key: 'X-Content-Type-Options', value: 'nosniff' }, { key: 'X-Frame-Options', value: 'DENY' }, { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, ]; } -module.exports = { buildCsp, buildConnectSrc, buildScriptSrc, securityHeaders }; +module.exports = { buildCsp, buildConnectSrc, securityHeaders }; diff --git a/web-ui/src/__tests__/security-headers.test.js b/web-ui/src/__tests__/security-headers.test.js index d0501710..8deb806f 100644 --- a/web-ui/src/__tests__/security-headers.test.js +++ b/web-ui/src/__tests__/security-headers.test.js @@ -49,14 +49,11 @@ describe('security headers (#657)', () => { expect(buildCsp({ NODE_ENV: 'development' })).toContain("'unsafe-eval'"); }); - test('securityHeaders ships the hardening header set', () => { - // The CSP moved OUT of this static set in #936: HTML documents get it from - // proxy.ts, which mints a per-request nonce. A static CSP cannot carry one, - // and two CSP headers on a response are intersected — so leaving it here - // would have blocked every script. Asserted explicitly below. + test('securityHeaders ships the CSP plus the hardening header set', () => { const keys = securityHeaders({}).map((h) => h.key); expect(keys).toEqual( expect.arrayContaining([ + 'Content-Security-Policy', 'X-Content-Type-Options', 'X-Frame-Options', 'Referrer-Policy', @@ -64,64 +61,3 @@ describe('security headers (#657)', () => { ); }); }); - -describe('production CSP carries a nonce, not unsafe-inline (#936)', () => { - const PROD = { NODE_ENV: 'production' }; - - /** The AC's CI check: the production policy must not allow inline script. */ - test('script-src has no unsafe-inline in production', () => { - const scriptSrc = buildCsp(PROD, { nonce: 'test-nonce' }) - .split('; ') - .find((d) => d.startsWith('script-src')); - - expect(scriptSrc).toBeDefined(); - expect(scriptSrc).not.toContain("'unsafe-inline'"); - }); - - test('script-src carries the request nonce and strict-dynamic', () => { - const scriptSrc = buildCsp(PROD, { nonce: 'abc123' }) - .split('; ') - .find((d) => d.startsWith('script-src')); - - expect(scriptSrc).toContain("'nonce-abc123'"); - expect(scriptSrc).toContain("'strict-dynamic'"); - }); - - test('production never ships unsafe-eval', () => { - expect(buildCsp(PROD, { nonce: 'n' })).not.toContain("'unsafe-eval'"); - }); - - test('development keeps unsafe-eval for the Next.js dev runtime', () => { - const csp = buildCsp({ NODE_ENV: 'development' }, { nonce: 'n' }); - expect(csp).toContain("'unsafe-eval'"); - // ...but still no unsafe-inline: the nonce works in dev too. - const scriptSrc = csp.split('; ').find((d) => d.startsWith('script-src')); - expect(scriptSrc).not.toContain("'unsafe-inline'"); - }); - - test('style-src deliberately keeps unsafe-inline', () => { - // Documented decision: Tailwind/React set style attributes directly, and a - // stolen style is not a stolen session. If this ever changes it should be a - // conscious edit, not a silent one. - const styleSrc = buildCsp(PROD, { nonce: 'n' }) - .split('; ') - .find((d) => d.startsWith('style-src')); - expect(styleSrc).toContain("'unsafe-inline'"); - }); - - test('the static header set no longer carries a CSP', () => { - // Two CSP headers are intersected by the browser, so a static (nonce-less) - // one alongside the proxy's would block every script. - const keys = securityHeaders(PROD).map((h) => h.key); - expect(keys).not.toContain('Content-Security-Policy'); - expect(keys).toContain('X-Content-Type-Options'); - }); - - test('a missing nonce still produces a working policy', () => { - // Non-document responses fall back to this; it must not be empty or broken. - const scriptSrc = buildCsp(PROD) - .split('; ') - .find((d) => d.startsWith('script-src')); - expect(scriptSrc).toContain("'self'"); - }); -}); diff --git a/web-ui/src/proxy.ts b/web-ui/src/proxy.ts deleted file mode 100644 index ebfc46a7..00000000 --- a/web-ui/src/proxy.ts +++ /dev/null @@ -1,59 +0,0 @@ -/** - * Per-request CSP nonce (#936). - * - * The production CSP used to ship `script-src 'self' 'unsafe-inline'`, and the - * security-headers comment conceded the consequence: with the JWT in - * localStorage and inline scripts allowed, any future HTML-injection bug became - * full session theft. connect-src/img-src closed fetch/XHR/img exfiltration, but - * nothing could close top-level navigation (`window.location = attacker + token`) - * once the injected script was already running. - * - * A nonce fixes the actual problem rather than the exfil channels: an injected - * inline script has no nonce, so it never executes. - * - * This must be a proxy (not a static `headers()` entry) because the nonce has to - * differ per request — a constant nonce is worth exactly as much as - * 'unsafe-inline'. - */ -import { NextResponse } from 'next/server'; -import type { NextRequest } from 'next/server'; - -// eslint-disable-next-line @typescript-eslint/no-var-requires -const { buildCsp } = require('../security-headers'); - -export function proxy(request: NextRequest) { - // crypto.getRandomValues: available on the Edge runtime, unlike node:crypto. - const bytes = new Uint8Array(16); - crypto.getRandomValues(bytes); - const nonce = btoa(String.fromCharCode(...bytes)); - - const csp = buildCsp(process.env, { nonce }); - - // Set on the REQUEST headers too: Next.js reads the nonce from the CSP header - // it receives and stamps it onto the framework's own inline bootstrap scripts. - // Without this the page's own scripts would be blocked by the policy we just - // set — the failure mode is a blank page, so it is not subtle. - const requestHeaders = new Headers(request.headers); - requestHeaders.set('x-nonce', nonce); - requestHeaders.set('Content-Security-Policy', csp); - - const response = NextResponse.next({ request: { headers: requestHeaders } }); - response.headers.set('Content-Security-Policy', csp); - return response; -} - -export const config = { - matcher: [ - { - // Everything except API routes, static assets and the favicon. Those are - // not HTML documents, so they need no nonce; next.config.js still applies - // the non-CSP hardening headers to them. - source: '/((?!api|_next/static|_next/image|favicon.ico).*)', - // Prefetches are not rendered, so nonce-ing them only costs work. - missing: [ - { type: 'header', key: 'next-router-prefetch' }, - { type: 'header', key: 'purpose', value: 'prefetch' }, - ], - }, - ], -}; From da5f11a58afdb2bb6fed83579f0f4aa186156116 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 10:35:26 -0700 Subject: [PATCH 5/9] fix(cli): stop --value-stdin leaking the secret, escape three more sites (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two PR-review findings, the first a leak this PR itself introduced. MAJOR — `--value-stdin` without `--provider` leaked the credential to the terminal. The interactive provider prompt runs typer.prompt(), which reads stdin, and it ran BEFORE the value read — so it consumed the piped secret as the provider choice, `choice_map.get(choice, choice)` assigned it to `provider`, and resolve_provider_name echoed it back in "Unknown provider: sk-ant-...". Exactly the exposure --value-stdin exists to prevent. Three changes, because one alone is not enough: - --provider is now required with --value-stdin/--value-file, checked before any prompt runs. - The non-interactive value is read FIRST, since stdin can only be consumed once and the provider prompt also reads it. - The unknown-provider error no longer echoes the rejected value at any of its four call sites — if stdin were ever mis-consumed, that value IS the secret. MINOR — `work update-description` printed the description unescaped, the same MarkupError crash this PR fixes elsewhere. Fixed, and replaced the point checks with a scanner test over cli/app.py that fails the build on any console.print f-string carrying task.title / blocker.question / description without escape(). The scanner immediately found two more sites the review had not named. Full tests/cli suite: 536 passed. --- codeframe/cli/app.py | 11 +++-- codeframe/cli/auth_commands.py | 73 +++++++++++++++++++++-------- tests/cli/test_cli_hardening_935.py | 52 ++++++++++++++++++++ 3 files changed, 113 insertions(+), 23 deletions(-) diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index 85f4b3a7..c80cb43a 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -3396,7 +3396,12 @@ def work_update_description( console.print("[green]Task description updated[/green]") console.print(f" Task: {escape(task.title)}") - console.print(f" New description: {description[:100]}{'...' if len(description) > 100 else ''}") + # escape(): the description is user input and Rich renders markup — + # "Array indexing a[0] and b[1]" would raise MarkupError (#935). + console.print( + f" New description: {escape(description[:100])}" + f"{'...' if len(description) > 100 else ''}" + ) console.print("\nNext steps:") console.print(f" codeframe work retry {task.id[:8]} # Retry with updated description") @@ -4995,7 +5000,7 @@ def blocker_show( console.print(f" Created: {blocker.created_at.strftime('%Y-%m-%d %H:%M')}") console.print("\n[bold]Question:[/bold]") - console.print(f" {blocker.question}") + console.print(f" {escape(blocker.question)}") if blocker.answer: console.print("\n[bold]Answer:[/bold]") @@ -5975,7 +5980,7 @@ def templates_show( deps = "" if task.depends_on_indices: deps = f" (depends on: {', '.join(str(d+1) for d in task.depends_on_indices)})" - console.print(f" {i}. {task.title}{deps}") + console.print(f" {i}. {escape(task.title)}{deps}") console.print(f" {task.description}") console.print(f" Est: {task.estimated_hours}h | Complexity: {task.complexity_score}/5 | Uncertainty: {task.uncertainty_level}") console.print() diff --git a/codeframe/cli/auth_commands.py b/codeframe/cli/auth_commands.py index d933ef77..a77584bb 100644 --- a/codeframe/cli/auth_commands.py +++ b/codeframe/cli/auth_commands.py @@ -581,6 +581,18 @@ def setup_credential( """ manager = CredentialManager() + # A non-interactive value source is incompatible with the interactive + # provider prompt: typer.prompt() reads stdin, so with `--value-stdin` and + # no --provider it would consume the piped SECRET as the provider choice and + # then echo it back in "Unknown provider: sk-ant-..." — leaking the exact + # thing this flag exists to protect (PR review on #935). + if (value_stdin or value_file is not None) and not provider: + console.print( + "[red]Error:[/red] --provider is required with --value-stdin/--value-file." + ) + console.print(" e.g. echo \"$KEY\" | codeframe auth setup -p github --value-stdin") + raise typer.Exit(1) + if value is not None: console.print( "[yellow]WARNING:[/yellow] --value puts the credential in this " @@ -589,6 +601,19 @@ def setup_credential( "or --value-file instead." ) + # Read the non-interactive value FIRST: stdin can only be consumed once, and + # the provider prompt below also reads it. + if value_stdin: + # .readline() not .read(): a trailing newline from `echo` is not part of + # the secret, and a here-doc may carry more than one line. + value = sys.stdin.readline().strip() + elif value_file is not None: + try: + value = value_file.read_text(encoding="utf-8").splitlines()[0].strip() + except (OSError, IndexError, UnicodeDecodeError) as exc: + console.print(f"[red]Error:[/red] could not read {value_file}: {exc}") + raise typer.Exit(1) + # Interactive provider selection if not provided if not provider: console.print("\n[bold]Select a provider to configure:[/bold]\n") @@ -610,22 +635,15 @@ def setup_credential( # Resolve provider name try: provider_enum = resolve_provider_name(provider) - except ValueError as e: - console.print(f"[red]Error:[/red] {e}") + except ValueError: + # Deliberately does NOT echo the rejected value: if stdin were ever + # mis-consumed the value would be the credential itself. + console.print( + "[red]Error:[/red] Unknown provider. Expected one of: " + "anthropic, openai, github, gitlab." + ) raise typer.Exit(1) - # stdin/file take precedence over the deprecated option. - if value_stdin: - # .readline() not .read(): a trailing newline from `echo` is not part of - # the secret, and a here-doc may carry more than one line. - value = sys.stdin.readline().strip() - elif value_file is not None: - try: - value = value_file.read_text(encoding="utf-8").splitlines()[0].strip() - except (OSError, IndexError, UnicodeDecodeError) as exc: - console.print(f"[red]Error:[/red] could not read {value_file}: {exc}") - raise typer.Exit(1) - # Prompt for value if not provided if not value: console.print(f"\nConfiguring [bold]{provider_enum.display_name}[/bold]") @@ -720,8 +738,13 @@ def validate_credential( # Resolve provider try: provider_enum = resolve_provider_name(provider) - except ValueError as e: - console.print(f"[red]Error:[/red] {e}") + except ValueError: + # Deliberately does NOT echo the rejected value: if stdin were ever + # mis-consumed the value would be the credential itself. + console.print( + "[red]Error:[/red] Unknown provider. Expected one of: " + "anthropic, openai, github, gitlab." + ) raise typer.Exit(1) # Get credential @@ -778,8 +801,13 @@ def rotate_credential( # Resolve provider try: provider_enum = resolve_provider_name(provider) - except ValueError as e: - console.print(f"[red]Error:[/red] {e}") + except ValueError: + # Deliberately does NOT echo the rejected value: if stdin were ever + # mis-consumed the value would be the credential itself. + console.print( + "[red]Error:[/red] Unknown provider. Expected one of: " + "anthropic, openai, github, gitlab." + ) raise typer.Exit(1) # Check if credential exists @@ -845,8 +873,13 @@ def remove_credential( # Resolve provider try: provider_enum = resolve_provider_name(provider) - except ValueError as e: - console.print(f"[red]Error:[/red] {e}") + except ValueError: + # Deliberately does NOT echo the rejected value: if stdin were ever + # mis-consumed the value would be the credential itself. + console.print( + "[red]Error:[/red] Unknown provider. Expected one of: " + "anthropic, openai, github, gitlab." + ) raise typer.Exit(1) # Check if credential exists in storage (not just environment) diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py index 9e6d1a2f..5ecc3eff 100644 --- a/tests/cli/test_cli_hardening_935.py +++ b/tests/cli/test_cli_hardening_935.py @@ -16,6 +16,7 @@ """ import inspect +import re from pathlib import Path import pytest @@ -147,6 +148,20 @@ def test_tasks_show_renders_a_hostile_title(self, workspace, title, tmp_path): assert result.exception is None, f"{title!r} crashed: {result.exception!r}" assert result.exit_code == 0 + def test_no_unescaped_user_text_reaches_console_print(self): + """Scanner, not a point check: a new console.print with user data should + fail the build rather than wait to be found in review.""" + source = (REPO_ROOT / "codeframe" / "cli" / "app.py").read_text() + offenders = [ + line.strip() + for line in source.splitlines() + if "console.print(f" in line + and re.search(r"\{(task\.title|blocker\.question|description)[^}]*\}", line) + and "escape(" not in line + ] + + assert not offenders, "unescaped user text in Rich output:\n" + "\n".join(offenders) + def test_markup_is_shown_literally_not_interpreted(self, workspace, tmp_path): tasks.create( workspace, title="Fix [/b] now", description="d", status=TaskStatus.READY @@ -165,6 +180,43 @@ def test_rich_would_have_raised_without_escaping(self): console.print(f"[cyan]Title:[/cyan] {HOSTILE_TITLES[0]}") +class TestStdinValueNeverLeaks: + """Raised by the PR bot: `--value-stdin` without `--provider` let the + interactive provider prompt consume the piped SECRET as the provider choice, + and the error path then echoed it back — leaking the exact thing the flag + exists to protect.""" + + def test_stdin_without_provider_is_rejected_before_any_prompt(self): + result = CliRunner().invoke( + app, ["auth", "setup", "--value-stdin"], input="sk-ant-SUPERSECRET\n" + ) + + assert result.exit_code == 1 + assert "--provider is required" in result.output + assert "SUPERSECRET" not in result.output, "the piped secret was echoed" + + def test_value_file_without_provider_is_rejected(self, tmp_path): + secret = tmp_path / "k" + secret.write_text("ghp_SUPERSECRET\n") + + result = CliRunner().invoke( + app, ["auth", "setup", "--value-file", str(secret)] + ) + + assert result.exit_code == 1 + assert "SUPERSECRET" not in result.output + + def test_an_unknown_provider_is_not_echoed_back(self): + """The rejected value could be a mis-consumed credential.""" + result = CliRunner().invoke( + app, ["auth", "setup", "--provider", "sk-ant-SUPERSECRET"] + ) + + assert result.exit_code == 1 + assert "SUPERSECRET" not in result.output + assert "Unknown provider" in result.output + + class TestTasksShowExists: """AC4 — README.md and CLAUDE.md advertise it.""" From 6b62f0583ffe1aac8f750704443113f37d9a42b1 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:01:58 -0700 Subject: [PATCH 6/9] fix(cli): broaden the markup scanner and escape the 17 sites it found (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review: `{task.description}` sat directly below the just-escaped `{task.title}` and the new scanner missed it. The regex enumerated `task.title|blocker.question|description` anchored right after `{`, so `{task.description}`, `{blocker.answer}` and `{t.title}` all slipped through — the reviewer named all three. The scanner now matches on the FIELD NAME rather than an enumerated set of variable names: any `{.title|.description|.question|.answer}` in a `console.print` f-string without escape() fails the build. A new variable name cannot slip past it. Rerunning it found 17 unescaped sites, not the 3 named — PRD titles, template descriptions, section titles, record titles. All user- or LLM-authored text rendered through Rich, all the same MarkupError crash. Escaped every one. This is the difference between a point fix and a scanner: the review found 3, the generalized check found 17. tests/cli: 536 passed. --- codeframe/cli/app.py | 54 ++++++++++++++--------------- tests/cli/test_cli_hardening_935.py | 8 ++++- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index c80cb43a..ce412808 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -482,7 +482,7 @@ def status( console.print("\n[bold]PRD[/bold]") latest_prd = prd.get_latest(workspace) if latest_prd: - console.print(f" Title: [green]{latest_prd.title}[/green]") + console.print(f" Title: [green]{escape(latest_prd.title)}[/green]") console.print(f" Added: {latest_prd.created_at.strftime('%Y-%m-%d %H:%M')}") else: console.print(" [dim]No PRD loaded. Run 'codeframe prd add '[/dim]") @@ -583,7 +583,7 @@ def summary( # PRD latest_prd = prd.get_latest(workspace) if latest_prd: - console.print(f"[bold]PRD:[/bold] {latest_prd.title}") + console.print(f"[bold]PRD:[/bold] {escape(latest_prd.title)}") else: console.print("[bold]PRD:[/bold] [dim]None[/dim]") @@ -819,7 +819,7 @@ def prd_templates_list() -> None: for template in templates: section_count = len(template.sections) console.print(f" [green]{template.id}[/green] - {template.name}") - console.print(f" {template.description}") + console.print(f" {escape(template.description)}") console.print(f" Sections: {section_count} | Version: {template.version}") console.print() @@ -850,13 +850,13 @@ def prd_templates_show( raise typer.Exit(1) console.print(f"\n[bold]{template.name}[/bold] ({template.id})\n") - console.print(f"{template.description}\n") + console.print(f"{escape(template.description)}\n") console.print(f"Version: {template.version}") console.print("\n[bold]Sections:[/bold]\n") for i, section in enumerate(template.sections, 1): required = "[green]required[/green]" if section.required else "[dim]optional[/dim]" - console.print(f" {i}. {section.title} ({section.id})") + console.print(f" {i}. {escape(section.title)} ({section.id})") console.print(f" Source: {section.source} | {required}") console.print() @@ -971,7 +971,7 @@ def prd_add( ) console.print("[green]PRD added[/green]") - console.print(f" Title: {record.title}") + console.print(f" Title: {escape(record.title)}") console.print(f" ID: {record.id}") console.print(f" Source: {file_path}") console.print() @@ -1027,7 +1027,7 @@ def prd_show( console.print("Add one with: codeframe prd add ") return - console.print(f"\n[bold]PRD:[/bold] {record.title}") + console.print(f"\n[bold]PRD:[/bold] {escape(record.title)}") console.print(f"[dim]ID: {record.id}[/dim]") console.print(f"[dim]Added: {record.created_at}[/dim]") @@ -1081,7 +1081,7 @@ def prd_list( console.print(f"\n[bold]PRDs ({len(records)}):[/bold]\n") for record in records: - console.print(f" {record.id[:8]}... [bold]{record.title}[/bold]") + console.print(f" {record.id[:8]}... [bold]{escape(record.title)}[/bold]") console.print(f" [dim]Added: {record.created_at.strftime('%Y-%m-%d %H:%M')}[/dim]") console.print() @@ -1152,7 +1152,7 @@ def prd_delete( print_event=False, ) - console.print(f"[green]PRD deleted:[/green] {record.title}") + console.print(f"[green]PRD deleted:[/green] {escape(record.title)}") except FileNotFoundError as e: console.print(f"[red]Error:[/red] {e}") @@ -1662,7 +1662,7 @@ def prd_generate( ) # Show result - console.print(f"\n[green]✓[/green] PRD generated: [bold]{prd_record.title}[/bold]") + console.print(f"\n[green]✓[/green] PRD generated: [bold]{escape(prd_record.title)}[/bold]") console.print(f"[dim]ID: {prd_record.id}[/dim]") # Show preview @@ -1781,7 +1781,7 @@ def prd_stress_test( console.print("[red]Error:[/red] No PRD found. Run 'codeframe prd generate' first.") raise typer.Exit(1) - console.print(f"[dim]Stress-testing PRD: {record.title} (v{record.version})[/dim]") + console.print(f"[dim]Stress-testing PRD: {escape(record.title)} (v{record.version})[/dim]") # Build provider: flag → env → config → anthropic, validating the # matching API key (#768) @@ -1960,7 +1960,7 @@ def tasks_generate( {t.id for t in tasks.list_tasks(workspace)} if overwrite else set() ) - console.print(f"Generating tasks from PRD: [bold]{prd_record.title}[/bold]") + console.print(f"Generating tasks from PRD: [bold]{escape(prd_record.title)}[/bold]") if recursive and no_llm: console.print( @@ -2355,7 +2355,7 @@ def tasks_set( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{actual_task_id}':") for t in matching: - console.print(f" {t.id}: {t.title[:40]}") + console.print(f" {t.id}: {escape(t.title[:40])}") console.print("Please provide a more specific ID.") raise typer.Exit(1) else: @@ -2493,7 +2493,7 @@ def tasks_delete( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching: - console.print(f" {t.id}: {t.title[:40]}") + console.print(f" {t.id}: {escape(t.title[:40])}") console.print("Please provide a more specific ID.") raise typer.Exit(1) @@ -2512,7 +2512,7 @@ def tasks_delete( f"[yellow]Warning:[/yellow] {len(dependents)} task(s) depend on this task" ) for dep in dependents[:3]: - console.print(f" - {dep.id[:8]}: {dep.title[:40]}") + console.print(f" - {dep.id[:8]}: {escape(dep.title[:40])}") if len(dependents) > 3: console.print(f" ... and {len(dependents) - 3} more") @@ -2662,7 +2662,7 @@ def work_start( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -2815,7 +2815,7 @@ def work_resume( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -2920,7 +2920,7 @@ def work_stop( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -3128,7 +3128,7 @@ def work_diagnose( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -3276,7 +3276,7 @@ def work_retry( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -3386,7 +3386,7 @@ def work_update_description( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -3469,7 +3469,7 @@ def work_follow( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{task_id}':") for t in matching[:5]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) task = matching[0] @@ -4097,7 +4097,7 @@ def batch_run( if len(matching) > 1: console.print(f"[red]Error:[/red] Multiple tasks match '{partial_id}':") for t in matching[:3]: - console.print(f" {t.id[:8]} - {t.title}") + console.print(f" {t.id[:8]} - {escape(t.title)}") raise typer.Exit(1) ids_to_execute.append(matching[0].id) else: @@ -5004,7 +5004,7 @@ def blocker_show( if blocker.answer: console.print("\n[bold]Answer:[/bold]") - console.print(f" {blocker.answer}") + console.print(f" {escape(blocker.answer)}") if blocker.answered_at: console.print(f" [dim]Answered: {blocker.answered_at.strftime('%Y-%m-%d %H:%M')}[/dim]") @@ -5943,7 +5943,7 @@ def templates_list( for template in templates: hours = template.total_estimated_hours console.print(f" [green]{template.id}[/green] - {template.name}") - console.print(f" {template.description}") + console.print(f" {escape(template.description)}") console.print(f" Category: {template.category} | Tasks: {len(template.tasks)} | Est: {hours:.1f}h") console.print() @@ -5969,7 +5969,7 @@ def templates_show( raise typer.Exit(1) console.print(f"\n[bold]{template.name}[/bold] ({template.id})\n") - console.print(f"{template.description}\n") + console.print(f"{escape(template.description)}\n") console.print(f"Category: {template.category}") console.print(f"Total Estimated Hours: {template.total_estimated_hours:.1f}h") if template.tags: @@ -5981,7 +5981,7 @@ def templates_show( if task.depends_on_indices: deps = f" (depends on: {', '.join(str(d+1) for d in task.depends_on_indices)})" console.print(f" {i}. {escape(task.title)}{deps}") - console.print(f" {task.description}") + console.print(f" {escape(task.description)}") console.print(f" Est: {task.estimated_hours}h | Complexity: {task.complexity_score}/5 | Uncertainty: {task.uncertainty_level}") console.print() diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py index 5ecc3eff..502e09f4 100644 --- a/tests/cli/test_cli_hardening_935.py +++ b/tests/cli/test_cli_hardening_935.py @@ -152,11 +152,17 @@ def test_no_unescaped_user_text_reaches_console_print(self): """Scanner, not a point check: a new console.print with user data should fail the build rather than wait to be found in review.""" source = (REPO_ROOT / "codeframe" / "cli" / "app.py").read_text() + # Match on the FIELD NAME, not on an enumerated set of variable names. + # The first version listed `task.title|blocker.question|description` + # anchored right after `{`, so it missed `{task.description}`, + # `{blocker.answer}` and `{t.title}` — the PR bot found all three. Any + # `{.title}` / `.description` / `.question` / `.answer` now + # counts, so a new variable name cannot slip past. offenders = [ line.strip() for line in source.splitlines() if "console.print(f" in line - and re.search(r"\{(task\.title|blocker\.question|description)[^}]*\}", line) + and re.search(r"\{[A-Za-z_][A-Za-z0-9_]*\.(title|description|question|answer)\b", line) and "escape(" not in line ] From 6952ee26d32cf5be797a4cd4d074f7265015ef36 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:11:42 -0700 Subject: [PATCH 7/9] fix(cli,tui): make the markup scanner statement-aware, escape 3 more sites (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review: req.title is rendered unescaped in tui/app.py's _update_proof_panel — the same MarkupError crash, missed because the scanner read only codeframe/cli/app.py. Extending it to the TUI was not enough: the scanner still passed. Those calls are written as log.write( f"... {req.title[:45]} | {gate_summary}" ) so the call name and the f-string are on DIFFERENT physical lines, and a line-based scan cannot see either half. It joins physical lines into logical ones by paren depth now, and covers console.print / log.write / add_row across both modules. That found a third site the reviewer had not named — a multi-line console.print of {s.description} in cli/app.py, invisible to every earlier version of this check. Second time this scanner has been too narrow: first it enumerated variable names (missed 17 sites), now it assumed one statement per line (missed 3). REQ titles are LLM-generated by the PRD stress test, so "Fix the [/b] parser" is realistic input, and the crash aborts every dashboard refresh. --- codeframe/cli/app.py | 2 +- codeframe/tui/app.py | 4 +- tests/cli/test_cli_hardening_935.py | 67 ++++++++++++++++++++++------- 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index ce412808..401bd455 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -3679,7 +3679,7 @@ def work_replay( for s in steps_to_show: status_color = {"completed": "green", "failed": "red"}.get(s.status, "yellow") console.print( - f"\n[bold]Step {s.step_number}:[/bold] {s.description} " + f"\n[bold]Step {s.step_number}:[/bold] {escape(s.description)} " f"[{status_color}][{s.status}][/{status_color}]" ) diff --git a/codeframe/tui/app.py b/codeframe/tui/app.py index 114614ff..e31a5aba 100644 --- a/codeframe/tui/app.py +++ b/codeframe/tui/app.py @@ -271,7 +271,7 @@ def _update_proof_panel(self, data: DashboardData) -> None: from datetime import date days = (req.waiver.expires - date.today()).days log.write( - f"[yellow bold]⚠ {req.id}[/yellow bold]: waiver expires in {days}d — {req.title[:50]}" + f"[yellow bold]⚠ {req.id}[/yellow bold]: waiver expires in {days}d — {escape(req.title[:50])}" ) for req in data.open_requirements: @@ -283,7 +283,7 @@ def _update_proof_panel(self, data: DashboardData) -> None: sev_color = self._SEVERITY_COLORS.get(req.severity.value, "white") log.write( f"[red]{req.id}[/red] [{sev_color}]{req.severity.value}[/{sev_color}]" - f" {req.title[:45]} | {gate_summary}" + f" {escape(req.title[:45])} | {gate_summary}" ) def action_refresh(self) -> None: diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py index 502e09f4..08568a99 100644 --- a/tests/cli/test_cli_hardening_935.py +++ b/tests/cli/test_cli_hardening_935.py @@ -148,25 +148,60 @@ def test_tasks_show_renders_a_hostile_title(self, workspace, title, tmp_path): assert result.exception is None, f"{title!r} crashed: {result.exception!r}" assert result.exit_code == 0 - def test_no_unescaped_user_text_reaches_console_print(self): - """Scanner, not a point check: a new console.print with user data should - fail the build rather than wait to be found in review.""" - source = (REPO_ROOT / "codeframe" / "cli" / "app.py").read_text() - # Match on the FIELD NAME, not on an enumerated set of variable names. - # The first version listed `task.title|blocker.question|description` - # anchored right after `{`, so it missed `{task.description}`, - # `{blocker.answer}` and `{t.title}` — the PR bot found all three. Any - # `{.title}` / `.description` / `.question` / `.answer` now - # counts, so a new variable name cannot slip past. + @pytest.mark.parametrize( + "module", + [ + "codeframe/cli/app.py", + # The TUI renders the same user text through RichLog/DataTable and + # was invisible to the first version of this scanner, which read only + # cli/app.py — the PR bot found req.title unescaped there. + "codeframe/tui/app.py", + ], + ) + def test_no_unescaped_user_text_reaches_rich_output(self, module): + """Scanner, not a point check: a new render of user data should fail the + build rather than wait to be found in review. + + Operates on whole *statements*, not lines. The line-based version missed + `log.write(\n f"... {req.title} ...")` in the TUI because the call and + the f-string sit on different lines — which is how the reviewer found two + sites the scanner had just declared clean. + """ + source = (REPO_ROOT / module).read_text() + + # Join physical lines into logical ones by tracking paren depth. The + # line-based version missed `log.write(\n f"... {req.title} ...")` in + # the TUI because the call and the f-string sit on different lines — + # which is how the reviewer found two sites the scanner had just + # declared clean. (Simple depth counting, not tokenize: 3.12 splits + # f-strings into separate tokens and the brace never survives.) + statements, buf, depth = [], "", 0 + for line in source.splitlines(): + stripped = line.strip() + buf = f"{buf} {stripped}" if buf else stripped + depth += line.count("(") - line.count(")") + if depth <= 0: + statements.append(buf) + buf, depth = "", 0 + + # Match on the FIELD NAME, not an enumerated set of variable names. The + # first version listed `task.title|blocker.question|description` anchored + # after `{`, so `{task.description}`, `{blocker.answer}` and `{t.title}` + # all slipped through. + field = re.compile(r"\{[A-Za-z_][A-Za-z0-9_]*\.(title|description|question|answer)\b") + renders = ("console.print", "log.write", "add_row") + offenders = [ - line.strip() - for line in source.splitlines() - if "console.print(f" in line - and re.search(r"\{[A-Za-z_][A-Za-z0-9_]*\.(title|description|question|answer)\b", line) - and "escape(" not in line + st for st in statements + if any(r in st for r in renders) + and field.search(st) + and "escape" not in st ] - assert not offenders, "unescaped user text in Rich output:\n" + "\n".join(offenders) + assert not offenders, ( + f"unescaped user text rendered through Rich in {module}:\n" + + "\n".join(o[:160] for o in offenders) + ) def test_markup_is_shown_literally_not_interpreted(self, workspace, tmp_path): tasks.create( From 4a3ed08e209165334d5a2603651f8cc5c672914b Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:35:51 -0700 Subject: [PATCH 8/9] fix(cli): expand the markup denylist, escape 24 more sites (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review, 4th round on this scanner: `cf prd stress-test` renders amb.label, amb.source_node_title and amb.recommendation unescaped — field names the scanner's list did not contain. LLM-generated ambiguity text, so "Fix the [/b] parser" is realistic input. Tried deny-by-default first (flag every interpolated attribute unless the final component is provably safe). It fires on 89 statements, nearly all timestamps, counts and enum accessors — churn, not safety. Reverted. The list now covers the free-prose fields that actually exist here: title, description, question, answer, label, recommendation, message, output, error, name, summary, content, text, reason, stderr, stdout, source_node_title, feedback, detail, hint, notes. That found 24 unescaped sites — the 3 named plus template names, check output, gate summaries, commit messages, checkpoint names, bottleneck recommendations and log messages. All escaped. The test now says in its own comment that it is a denylist and why the alternatives were rejected. #1054 filed (P3.8) to replace it with a real type-aware lint rule; a field-name list cannot distinguish prose from an int and will keep drifting. Scanner history in one PR: variable names (missed 17) -> field names line-based (missed 3 + a whole module) -> statement-aware (missed 3 field names) -> this. --- codeframe/cli/app.py | 48 ++++++++++++++--------------- tests/cli/test_cli_hardening_935.py | 20 +++++++++--- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index 401bd455..14c2ea39 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -218,7 +218,7 @@ def init( if hook_result.success: console.print(f" Hook after_init: [green]OK[/green] ({hook_result.duration_ms}ms)") else: - console.print(f" Hook after_init: [yellow]failed[/yellow] ({hook_result.stderr[:100]})") + console.print(f" Hook after_init: [yellow]failed[/yellow] ({escape(hook_result.stderr[:100])})") # Generate CODEFRAME.md if requested (never clobber an existing one, #778) if generate_config: @@ -682,11 +682,11 @@ def review( status_str = "[yellow]ERROR[/yellow]" duration_str = f" ({check.duration_ms}ms)" if check.duration_ms else "" - console.print(f" {check.name}: {status_str}{duration_str}") + console.print(f" {escape(check.name)}: {status_str}{duration_str}") # Show output for failures or verbose mode if check.output and (check.status == GateStatus.FAILED or verbose): - console.print(f" [dim]{check.output}[/dim]") + console.print(f" [dim]{escape(check.output)}[/dim]") # Diagnostics that are not gate verdicts (#909) — e.g. a failed # dependency install. Always shown: the gates may all have passed, and @@ -697,9 +697,9 @@ def review( # Summary console.print() if result.passed: - console.print(f"[bold green]All gates passed[/bold green] ({result.summary})") + console.print(f"[bold green]All gates passed[/bold green] ({escape(result.summary)})") else: - console.print(f"[bold red]Gates failed[/bold red] ({result.summary})") + console.print(f"[bold red]Gates failed[/bold red] ({escape(result.summary)})") raise typer.Exit(1) except FileNotFoundError: @@ -818,7 +818,7 @@ def prd_templates_list() -> None: console.print("\n[bold]Available PRD Templates:[/bold]\n") for template in templates: section_count = len(template.sections) - console.print(f" [green]{template.id}[/green] - {template.name}") + console.print(f" [green]{template.id}[/green] - {escape(template.name)}") console.print(f" {escape(template.description)}") console.print(f" Sections: {section_count} | Version: {template.version}") console.print() @@ -849,7 +849,7 @@ def prd_templates_show( console.print(f" {t.id}") raise typer.Exit(1) - console.print(f"\n[bold]{template.name}[/bold] ({template.id})\n") + console.print(f"\n[bold]{escape(template.name)}[/bold] ({template.id})\n") console.print(f"{escape(template.description)}\n") console.print(f"Version: {template.version}") @@ -916,7 +916,7 @@ def prd_templates_import( try: # Import and persist to project directory template = manager.import_template(source_path, persist=True) - console.print(f"[green]✓[/green] Imported template '{template.id}' ({template.name})") + console.print(f"[green]✓[/green] Imported template '{template.id}' ({escape(template.name)})") console.print(f"[dim]Sections: {len(template.sections)}[/dim]") console.print(f"[dim]Saved to: .codeframe/templates/prd/{template.id}.yaml[/dim]") except Exception as e: @@ -1504,10 +1504,10 @@ def prd_generate( console.print(f"[red]Error:[/red] Template '{template}' not found.") console.print("\nAvailable templates:") for t in template_manager.list_templates(): - console.print(f" {t.id} - {t.name}") + console.print(f" {t.id} - {escape(t.name)}") raise typer.Exit(1) - console.print(f"[dim]Using template: {template_obj.name}[/dim]") + console.print(f"[dim]Using template: {escape(template_obj.name)}[/dim]") try: workspace = get_workspace(workspace_path) @@ -1805,11 +1805,11 @@ def prd_stress_test( if result.ambiguities: console.print(f"\n[bold yellow]⚠ {len(result.ambiguities)} ambiguities found[/bold yellow]\n") for i, amb in enumerate(result.ambiguities, 1): - console.print(f"[bold yellow]{i}. {amb.label}[/bold yellow] (from \"{amb.source_node_title}\")") + console.print(f"[bold yellow]{i}. {escape(amb.label)}[/bold yellow] (from \"{escape(amb.source_node_title)}\")") console.print(" The PRD doesn't specify:") for q in amb.questions: console.print(f" [cyan]- {q}[/cyan]") - console.print(f" → {amb.recommendation}") + console.print(f" → {escape(amb.recommendation)}") console.print() else: console.print("\n[green]✓ No ambiguities found — PRD is well-specified.[/green]\n") @@ -1818,7 +1818,7 @@ def prd_stress_test( if interactive and result.ambiguities: console.print("[bold]Interactive mode — resolve ambiguities:[/bold]\n") for amb in result.ambiguities: - console.print(f"[yellow]{amb.label}[/yellow]: {', '.join(amb.questions)}") + console.print(f"[yellow]{escape(amb.label)}[/yellow]: {', '.join(amb.questions)}") answer = typer.prompt("Your answer") amb.resolved_answer = answer console.print("[green]✓[/green] Recorded.\n") @@ -2736,7 +2736,7 @@ def work_start( if state.step_results: last_result = state.step_results[-1] if last_result.error: - console.print(f" Error: {last_result.error[:200]}") + console.print(f" Error: {escape(last_result.error[:200])}") # Show debug log location if debugging was enabled if debug: @@ -3204,7 +3204,7 @@ def _display_diagnostic_report( console.print("\n[bold]Recommendations:[/bold]\n") for i, rec in enumerate(report.recommendations, 1): console.print(f" {i}. [cyan]{rec.action.value}[/cyan]") - console.print(f" {rec.reason}") + console.print(f" {escape(rec.reason)}") console.print(f" [dim]Command:[/dim] [green]{rec.command}[/green]") console.print() @@ -3220,7 +3220,7 @@ def _display_diagnostic_report( if logs: console.print(f"\n[bold]Recent Errors ({len(logs)}):[/bold]") for log in logs[:5]: - console.print(f" [red]ERROR[/red] {log.category.value}: {log.message[:100]}") + console.print(f" [red]ERROR[/red] {log.category.value}: {escape(log.message[:100])}") console.print("━" * 60) console.print(f"[dim]Report ID: {report.id}[/dim]") @@ -4209,7 +4209,7 @@ def batch_run( console.print("\n[bold]Code Review Results[/bold]") for check in gate_result.checks: status_color = "green" if check.status.value == "PASSED" else "red" - console.print(f" [{status_color}]{check.name}[/{status_color}]: {check.status.value}") + console.print(f" [{status_color}]{escape(check.name)}[/{status_color}]: {check.status.value}") if check.output and check.status.value != "PASSED": # Show truncated output for failures output_lines = check.output.strip().split("\n")[:5] @@ -5341,7 +5341,7 @@ def commit_create( console.print("\n[bold green]Commit created[/bold green]") console.print(f" Hash: {commit_info.hash}") - console.print(f" Message: {commit_info.message}") + console.print(f" Message: {escape(commit_info.message)}") console.print( f" Changes: {commit_info.files_changed} files, " f"+{commit_info.insertions}/-{commit_info.deletions}" @@ -5393,7 +5393,7 @@ def checkpoint_create( summary = checkpoint.snapshot.get("summary", {}) console.print("\n[bold green]Checkpoint created[/bold green]") - console.print(f" Name: {checkpoint.name}") + console.print(f" Name: {escape(checkpoint.name)}") console.print(f" ID: [dim]{checkpoint.id[:8]}[/dim]") console.print(f" Tasks: {summary.get('total_tasks', 0)}") @@ -5491,7 +5491,7 @@ def checkpoint_show( summary = checkpoint.snapshot.get("summary", {}) tasks_by_status = summary.get("tasks_by_status", {}) - console.print(f"\n[bold]Checkpoint:[/bold] {checkpoint.name}") + console.print(f"\n[bold]Checkpoint:[/bold] {escape(checkpoint.name)}") console.print(f" ID: {checkpoint.id}") console.print(f" Created: {checkpoint.created_at.strftime('%Y-%m-%d %H:%M:%S')}") @@ -5544,7 +5544,7 @@ def checkpoint_restore( summary = checkpoint.snapshot.get("summary", {}) console.print("\n[bold green]Checkpoint restored[/bold green]") - console.print(f" Name: {checkpoint.name}") + console.print(f" Name: {escape(checkpoint.name)}") console.print(f" Tasks restored: {summary.get('total_tasks', 0)}") except FileNotFoundError: @@ -5884,7 +5884,7 @@ def schedule_bottlenecks( console.print(f"[yellow]Task {bn.task_id}:[/yellow] {title}") console.print(f" Type: {bn.bottleneck_type}") console.print(f" Impact: {bn.impact_hours:.1f} hours") - console.print(f" Recommendation: {bn.recommendation}") + console.print(f" Recommendation: {escape(bn.recommendation)}") console.print() except FileNotFoundError as e: @@ -5942,7 +5942,7 @@ def templates_list( console.print("\n[bold]Available Templates:[/bold]\n") for template in templates: hours = template.total_estimated_hours - console.print(f" [green]{template.id}[/green] - {template.name}") + console.print(f" [green]{template.id}[/green] - {escape(template.name)}") console.print(f" {escape(template.description)}") console.print(f" Category: {template.category} | Tasks: {len(template.tasks)} | Est: {hours:.1f}h") console.print() @@ -5968,7 +5968,7 @@ def templates_show( console.print(f" {t.id}") raise typer.Exit(1) - console.print(f"\n[bold]{template.name}[/bold] ({template.id})\n") + console.print(f"\n[bold]{escape(template.name)}[/bold] ({template.id})\n") console.print(f"{escape(template.description)}\n") console.print(f"Category: {template.category}") console.print(f"Total Estimated Hours: {template.total_estimated_hours:.1f}h") diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py index 08568a99..47dff9b5 100644 --- a/tests/cli/test_cli_hardening_935.py +++ b/tests/cli/test_cli_hardening_935.py @@ -184,11 +184,21 @@ def test_no_unescaped_user_text_reaches_rich_output(self, module): statements.append(buf) buf, depth = "", 0 - # Match on the FIELD NAME, not an enumerated set of variable names. The - # first version listed `task.title|blocker.question|description` anchored - # after `{`, so `{task.description}`, `{blocker.answer}` and `{t.title}` - # all slipped through. - field = re.compile(r"\{[A-Za-z_][A-Za-z0-9_]*\.(title|description|question|answer)\b") + # A denylist of free-text FIELD names — deliberately, after trying the + # alternatives. Enumerating variable names failed (missed 17 sites); + # deny-by-default on every attribute over-fires on 89 statements that + # interpolate timestamps, counts and enum accessors, which would be + # churn rather than safety. This list covers the free-prose fields that + # actually exist in this codebase; a real lint rule is the durable + # answer and is filed as a follow-up. + FREE_TEXT = ( + "title|description|question|answer|label|recommendation|message|" + "output|error|name|summary|content|text|reason|stderr|stdout|" + "source_node_title|feedback|detail|hint|notes" + ) + field = re.compile( + r"\{[A-Za-z_][A-Za-z0-9_]*\.(?:" + FREE_TEXT + r")\b" + ) renders = ("console.print", "log.write", "add_row") offenders = [ From 7b887766ef48c1ae9c3017ba7537d393d598e7fc Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sun, 2 Aug 2026 11:42:39 -0700 Subject: [PATCH 9/9] fix(cli): escape joined questions; match fields inside any interpolation (#935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review, 5th round: `', '.join(amb.questions)` was still unescaped, and the scanner missed it for TWO independent reasons. 1. The check was per-STATEMENT — `"escape" not in st` — so once the line contained one escape() call (amb.label, fixed last round), the whole statement was skipped and any other interpolation on it went unexamined. Now each `{...}` span is checked on its own. 2. The field regex required the field at the start of the interpolation (`{obj.field}`), so `{', '.join(amb.questions)}` did not match. It now looks for the field anywhere inside the span. And a third, found by fixing those two: the field list has `question`, the attribute is `questions`, and `\b` after the singular refuses to match the plural. Added `s?`. Scanner history across this one PR: variable names -> field names line-based -> statement-aware -> expanded field list -> per-span with plurals. Each round the reviewer found 1-3 sites and the generalization found more. #1054 (P3.8) tracks replacing it with a type-aware lint rule, which is the only version of this that stops needing another round. --- codeframe/cli/app.py | 2 +- tests/cli/test_cli_hardening_935.py | 20 +++++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index 14c2ea39..d78efb3f 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -1818,7 +1818,7 @@ def prd_stress_test( if interactive and result.ambiguities: console.print("[bold]Interactive mode — resolve ambiguities:[/bold]\n") for amb in result.ambiguities: - console.print(f"[yellow]{escape(amb.label)}[/yellow]: {', '.join(amb.questions)}") + console.print(f"[yellow]{escape(amb.label)}[/yellow]: {escape(', '.join(amb.questions))}") answer = typer.prompt("Your answer") amb.resolved_answer = answer console.print("[green]✓[/green] Recorded.\n") diff --git a/tests/cli/test_cli_hardening_935.py b/tests/cli/test_cli_hardening_935.py index 47dff9b5..c45f4fef 100644 --- a/tests/cli/test_cli_hardening_935.py +++ b/tests/cli/test_cli_hardening_935.py @@ -196,16 +196,26 @@ def test_no_unescaped_user_text_reaches_rich_output(self, module): "output|error|name|summary|content|text|reason|stderr|stdout|" "source_node_title|feedback|detail|hint|notes" ) - field = re.compile( - r"\{[A-Za-z_][A-Za-z0-9_]*\.(?:" + FREE_TEXT + r")\b" - ) + # The field may appear ANYWHERE inside the interpolation, not only as a + # bare `{obj.field}` — `{', '.join(amb.questions)}` was the fifth gap + # found in this review cycle. Match `{...}` spans, then look inside. + interpolation = re.compile(r"\{[^{}]*\}") + # `s?`: the attribute is often plural (`amb.questions`), and \b after a + # singular name refuses to match it — which is why the reviewer found + # `', '.join(amb.questions)` still unescaped. + free_field = re.compile(r"\.(?:" + FREE_TEXT + r")s?\b") + + def _has_unescaped_field(statement: str) -> bool: + return any( + free_field.search(span) and "escape(" not in span + for span in interpolation.findall(statement) + ) renders = ("console.print", "log.write", "add_row") offenders = [ st for st in statements if any(r in st for r in renders) - and field.search(st) - and "escape" not in st + and _has_unescaped_field(st) ] assert not offenders, (