fix(cli): bind loopback, keep secrets off argv, escape markup, add tasks show (#935) - #1049
fix(cli): bind loopback, keep secrets off argv, escape markup, add tasks show (#935)#1049frankbria wants to merge 5 commits into
Conversation
…sks show (#935) 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/<pid>/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 <id>` 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.
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.
|
Claude finished @frankbria's task in 5m 45s —— View job GLM precision review — CLI hardening (#935)Bug-hunting review (logic errors, security, data loss, crashes only). Style/architecture/coverage handled by a separate review.
Result — 1 defect foundmajor — secret exposure in the new Checked and clean: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # 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() |
There was a problem hiding this comment.
[major] --value-stdin without --provider leaks the credential to the terminal — the very exposure this PR is meant to prevent.
The interactive provider prompt at line 604 (typer.prompt(...)) reads from stdin and runs before this read, so it consumes the piped secret as the provider choice. choice_map.get(choice, choice) then assigns the secret to provider, and resolve_provider_name raises ValueError(f"Unknown provider: '{name}' ...") — echoing the name verbatim. The except ValueError at line 613 prints that message via console.print(f"[red]Error:[/red] {e}"), so the secret is written to stdout in plaintext. (Independent of the leak, --value-stdin also simply cannot work without -p, because the provider prompt eats the first stdin line.)
Failure scenario: echo "$MY_TOKEN" | codeframe auth setup --value-stdin (no -p) → terminal/CI log shows Error: Unknown provider: 'ghp_xxxx...'. Valid providers: ..., and no credential is stored.
| # 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() | |
| # stdin/file take precedence over the deprecated option. The value must be | |
| # read (or the option rejected) before the interactive provider prompt: | |
| # typer.prompt() reads stdin too, so `echo $KEY | cf auth setup --value-stdin` | |
| # with no -p would consume the secret as the provider name and echo it back | |
| # in the ValueError raised below (#935). | |
| if (value_stdin or value_file is not None) and not provider: | |
| console.print( | |
| "[red]Error:[/red] --value-stdin/--value-file require --provider/-p " | |
| "(provider selection also reads stdin)." | |
| ) | |
| raise typer.Exit(1) | |
| if value_stdin: | |
| value = sys.stdin.readline().strip() |
|
GLM precision review — scanned for concrete defects only (logic errors, crashes, secret exposure, data loss, race conditions). Style/architecture/coverage are out of scope and handled separately.
Checked and clean:
Minor non-defect noted (not blocking): |
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.
GLM precision review — CLI hardening (#935)
|
GLM precision review — CLI hardening (#935)
|
|
Claude finished @frankbria's task in 8m 49s —— View job GLM precision review — CLI hardening (#935)Re-review on synchronize. Bug-hunting (logic errors, security, data loss, crashes only); style/architecture/coverage handled separately.
Result — 1 new defect + 1 prior finding still open
Verified clean: Summary posted via |
| 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 ''}") |
There was a problem hiding this comment.
[minor] The description argument is echoed into console.print unescaped — a MarkupError crash on legitimate input, the same bug class this PR fixes for titles/blockers elsewhere.
Failure scenario: codeframe work update-description <id> "Fix the [/b] parser" (or ordinary prose like "Array indexing a[0] and b[1]", which the PR's own test asserts Rich chokes on). The line above (3398) was escaped in this PR and tasks_show escapes task.description — so descriptions are treated as needing escape elsewhere — but this echo was missed. The crash also fires after tasks_module.update(...) (line 3395) already persisted the new description, so the operator sees an error while the update silently succeeded.
| console.print(f" New description: {description[:100]}{'...' if len(description) > 100 else ''}") | |
| console.print(f" New description: {escape(description[:100])}{'...' if len(description) > 100 else ''}") |
Same crash class nearby in this function's branches (not changed by the PR, but reachable with LLM-generated titles / user-typed ids): {t.title} at line 3389 and {task_id} at lines 3383/3387 are also interpolated unescaped.
GLM precision review — CLI hardening (#935)Re-review on synchronize. Scanned for concrete defects only (logic errors, crashes, secret exposure, data loss, race conditions). Style/architecture/coverage are out of scope and handled separately.
Checked and clean:
Non-blocking note (not a defect): |
1 participant
Closes #935.
1. The server was on the LAN by default
cf serve --hostandrun_serverboth defaulted to0.0.0.0while the printed hints saidlocalhost. That process exposes SQLite state, workspace file access and agent-execution endpoints, so the default put all of it on the local network.Both now default to
127.0.0.1, and.env.exampleno longer suggests0.0.0.0. Choosing a wildcard bind warns:and warns much harder when auth is off — the combination the issue calls out, and one the dev docs actively suggest:
2. Credentials in argv
auth setup --value/-vput the secret in the process command line — readable by any local process via/proc/<pid>/cmdlinefor the lifetime of the call, and written to shell history. The docstring taught it, with real-looking keys:--valueis nowhidden=True, marked DEPRECATED in its help, and prints a warning when used. The supported non-interactive paths are:Kept rather than removed outright so existing scripts do not break silently — they get a warning instead.
--value-stdinusesreadline(), so a trailing newline fromechois not stored as part of the secret.3. A task title could crash the CLI
Titles and blocker text were interpolated into Rich-rendered output with markup enabled:
cf tasks listand the TUI blocker log crashed on a perfectly legitimate title. Every user-controlled interpolation incli/app.pyandtui/app.pyis nowescape()d.Tested by rendering five hostile-but-legitimate titles through the real
CliRunner— includingArray indexing a[0] and b[1], which is ordinary prose that Rich also chokes on. One extra test asserts Rich would raise on those titles unescaped, so the suite cannot pass vacuously.4.
cf tasks showdid not existAdvertised in both
README.md("Task details with dependencies") andCLAUDE.md. Implemented rather than deleting a documented feature: details, dependencies with resolved titles, accepts a unique ID prefix, and exits non-zero with a helpful message on an unknown or ambiguous id.Acceptance criteria
cf serveand the server default to 127.0.0.1; 0.0.0.0 requires an explicit flag and warns when auth is disabled[/b]renders without raisingcf tasks show <id>existsTests
29 tests, 18 verified RED against
main.ruffandmypy codeframe/(197 files) clean.Known limitations
--valueis deprecated, not removed. Removing it outright would break existing automation with no migration window; it is hidden from help and warns on use.cli/app.pyandtui/app.py. Aconsole.printadded elsewhere later could reintroduce the bug — a lint for "f-string with user data into console.print" would generalize this, but is beyond this issue.typer.confirmoutput is not Rich-rendered, so those call sites are intentionally left unescaped (noted inline).