[P0] cortex mcp-server --verify (CLI audit P0-2) - #52
Conversation
469d82b to
21713ba
Compare
Offline stdio JSON-RPC verifier (cortex-verify/1) for TUI chrome, lock scenes, login product copy, and API error paths. Hidden until Designer sign-off. CLI_100_AUDIT_READY + CLI_100_CHROME_LOCK_SIGNED. Co-authored-by: Mathis <echobt@users.noreply.github.com>
Extract mcp-server dispatch from oversized modules, regenerate the CLI schema, and add unit tests for the hidden verify paths. mcp-server stays hide=true. Co-authored-by: Mathis <echobt@users.noreply.github.com>
21713ba to
b40fb5d
Compare
Greptile SummaryThis PR adds the hidden
Confidence Score: 0/5The PR is not safe to merge because MCP inputs can escape filesystem boundaries or terminate the verifier, and multiple audit tools can report broken behavior as successful. Unvalidated report and resource paths allow writes and reads outside their documented directories; unchecked coordinates and scene IDs can panic the inline stdio server; and TUI, API, lock, assertion, and login paths contain concrete false-positive verification behavior. Files Needing Attention: src/cortex-cli/src/verify_mcp/report.rs, src/cortex-cli/src/verify_mcp/resources.rs, src/cortex-cli/src/verify_mcp/tui.rs, src/cortex-cli/src/verify_mcp/lock.rs, src/cortex-cli/src/verify_mcp/api.rs, src/cortex-cli/src/verify_mcp/login.rs
|
| Filename | Overview |
|---|---|
| src/cortex-cli/src/verify_mcp/report.rs | Adds report persistence, but the unvalidated report ID permits writes outside the intended directory. |
| src/cortex-cli/src/verify_mcp/resources.rs | Adds verification resources, but crafted lock URIs can disclose arbitrary readable local files. |
| src/cortex-cli/src/verify_mcp/tui.rs | Adds headless TUI tools, but key handling diverges from production, assertions can falsely pass, and unchecked cell coordinates can panic. |
| src/cortex-cli/src/verify_mcp/api.rs | Adds API verification tools, but client errors and unconsumed streams can be reported as successful. |
| src/cortex-cli/src/verify_mcp/lock.rs | Adds lock rendering and palette tools, but unknown IDs can panic and failed legend checks are recorded as passing. |
| src/cortex-cli/src/verify_mcp/login.rs | Adds product-copy login fixtures, including an ok fixture that reports success without exercising login. |
| src/cortex-cli/src/verify_mcp/mod.rs | Registers and dispatches the verifier's tools and resources; several unrestricted schemas expose the affected paths. |
| src/cortex-cli/tests/mcp_server_verify.rs | Covers the happy-path stdio contract but does not exercise traversal, invalid coordinates/IDs, or false-success cases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[MCP client over stdio] --> Server[cortex-verify server]
Server --> Dispatch[Tool dispatch]
Server --> Resources[Resource provider]
Dispatch --> TUI[Headless TUI sessions]
Dispatch --> Lock[Production lock renderers]
Dispatch --> API[API and login probes]
Dispatch --> Peer[Configured MCP peers]
Dispatch --> Report[Report generation]
Resources --> Fixtures[Checked-in lock fixtures]
Resources --> Latest[Latest report state]
Report --> Disk[target/readiness/cli-verify]
Reviews (1): Last reviewed commit: "fix(cli): green verify mcp source policy..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let path = report_path(run_id); | ||
| if let Some(parent) = path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| std::fs::write(&path, serde_json::to_string_pretty(&value)?)?; |
There was a problem hiding this comment.
report.finish accepts any run_id, appends .json, and joins it directly beneath the report directory. A value such as /tmp/verify-output or ../../outside makes create_dir_all and write operate outside target/readiness/cli-verify, allowing an MCP client to create or overwrite writable JSON files elsewhere.
How this was verified: The client-controlled tool argument reaches
PathBuf::joinandstd::fs::writewithout component validation or a containment check.
| if let Some(rest) = uri.strip_prefix("cortex-verify://lock/v2/") { | ||
| let path = workspace_root() | ||
| .join("docs/media/tui-lock-v2/txt") | ||
| .join(rest); | ||
| let text = | ||
| std::fs::read_to_string(&path).map_err(|_| anyhow!("Resource not found: {uri}"))?; | ||
| return Ok(ResourceContent::text(uri, text)); |
There was a problem hiding this comment.
The lock resource reader accepts any URI with the expected prefix and joins the remaining text directly onto the fixture directory. A request such as cortex-verify://lock/v2/../../Cargo.toml traverses outside that directory, while a suffix beginning with / replaces the base path. The selected file is then returned to the MCP client. Restrict reads to advertised scene resources or verify that the resolved path remains inside the fixture directory.
How this was verified: Arbitrary JSON-RPC resource URIs are passed to this provider, where their unvalidated suffix reaches
read_to_stringand the resulting contents are returned.
| "cell" => { | ||
| let x = check.get("x").and_then(Value::as_u64).unwrap_or(0) as u16; | ||
| let y = check.get("y").and_then(Value::as_u64).unwrap_or(0) as u16; | ||
| let cell = &frame.buffer[(x, y)]; |
There was a problem hiding this comment.
A tui.assert request can provide coordinates outside the captured terminal, but the cell assertion indexes the ratatui buffer directly. For example, x=40 against a 40-column session panics instead of returning a failed check or tool error. Because the stdio request loop awaits the handler without panic isolation, this terminates the verifier rather than producing a JSON-RPC response.
| .ok_or_else(|| anyhow::anyhow!("id is required"))?; | ||
| let width = args.get("width").and_then(Value::as_u64).unwrap_or(120) as u16; | ||
| let height = args.get("height").and_then(Value::as_u64).unwrap_or(40) as u16; | ||
| let frame = render_pack(pack, id, width, height)?; |
There was a problem hiding this comment.
lock.render and lock.diff_txt pass the unrestricted MCP id into render_lock_v2_scene, whose fallback arm panics for unknown IDs. A typo or crafted scene name therefore unwinds the inline stdio request loop and kills the verifier instead of returning a tool error. Validate the ID against the selected scene list before invoking the renderer.
| fn apply_key(session: &mut TuiSession, name: &str) -> Result<()> { | ||
| let event = parse_key_string(name).ok_or_else(|| anyhow::anyhow!("unknown key {name}"))?; | ||
| let action = session.mapper.get_action(event, ActionContext::Input); | ||
| match action { | ||
| KeyAction::Clear => session.app_state.input.set_text(""), | ||
| KeyAction::NewLine => session.app_state.input.insert_str("\n"), | ||
| _ if name.eq_ignore_ascii_case("Backspace") => { | ||
| let text = session.app_state.input.text(); | ||
| let mut chars: Vec<char> = text.chars().collect(); | ||
| chars.pop(); | ||
| session | ||
| .app_state | ||
| .input | ||
| .set_text(&chars.into_iter().collect::<String>()); | ||
| } | ||
| _ if name.chars().count() == 1 => session.app_state.input.insert_str(name), | ||
| _ => {} |
There was a problem hiding this comment.
The TUI key verifier does not drive the production EventLoop: startup discards the loop, and apply_key hardcodes ActionContext::Input while silently ignoring almost every mapped action. Checks that press Enter to submit, navigate history or autocomplete, move the caret, or handle approval and sidebar keys consequently leave state unchanged. The verifier can therefore pass while the production key path is broken.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| state.record_flow(FlowRow { | ||
| id: "api.me".into(), | ||
| status: if status < 500 { "pass" } else { "fail" }.into(), | ||
| checks: vec![Check { | ||
| name: "whoami".into(), | ||
| ok: status < 500, | ||
| detail: Some(format!("status={status}")), |
There was a problem hiding this comment.
api.me records every response below 500 as a successful flow. Authentication failures, missing endpoints, and rate limits such as 401, 404, and 429 therefore make the verification report greener, even though these statuses are failures under the actual CLI client contract. They must not increment the report's pass count.
| match client.stream_turn(message, mode).await { | ||
| Ok(_stream) => { | ||
| state.record_flow(FlowRow { | ||
| id: "api.turn".into(), | ||
| status: "pass".into(), | ||
| checks: vec![Check { | ||
| name: "stream".into(), | ||
| ok: true, | ||
| detail: None, | ||
| }], | ||
| }); | ||
| Ok(json!({ | ||
| "events": [], | ||
| "stopped_once": false, | ||
| })) |
There was a problem hiding this comment.
api.turn declares the audit successful as soon as a 2xx response produces a stream object, then drops that stream without consuming any events. A server that returns 2xx and immediately closes, emits an error, or sends malformed SSE is therefore reported as passing with events: []. Consume the stream through the behavior being audited before recording success.
| let legend_ok = frame.plain.contains("/ commands") && frame.plain.contains("@ files"); | ||
| state.record_state(StateRow { | ||
| id: id.to_string(), | ||
| pack: pack.to_string(), | ||
| size: [width, height], | ||
| status: "pass".into(), | ||
| frame_sha256: Some(super::frame::sha256_hex(frame.plain.as_bytes())), | ||
| checks: vec![Check { | ||
| name: "legend_complete".into(), | ||
| ok: legend_ok, | ||
| detail: None, |
There was a problem hiding this comment.
lock.render computes whether the required legend is present but always records the state as pass. Report counters and report.finish derive the exit code only from row status, so a truncated or missing legend leaves summary.fail unchanged and produces exit code 0. This masks the regression that the legend check is intended to detect.
| "no_color" => { | ||
| let rgb = check.get("rgb").and_then(Value::as_str).unwrap_or(""); | ||
| let counts = count_palette(&frame.buffer); | ||
| let ok = match rgb.to_ascii_uppercase().as_str() { | ||
| "#A78BFA" => counts.violet_px == 0, | ||
| "#221A38" => counts.wash_px == 0, | ||
| "#C9A95C" => counts.gold_px == 0, | ||
| "#00F5D4" => counts.mint_px == 0, | ||
| _ => true, | ||
| }; | ||
| Check { | ||
| name: format!("no_color:{rgb}"), | ||
| ok, | ||
| detail: (!ok).then(|| format!("found {rgb}")), | ||
| } | ||
| } | ||
| "row" => { | ||
| let y = check.get("y").and_then(Value::as_u64).unwrap_or(0) as usize; | ||
| let eq = check.get("eq").and_then(Value::as_str).unwrap_or(""); | ||
| let row = frame.plain.lines().nth(y).unwrap_or(""); | ||
| let ok = row == eq; | ||
| Check { | ||
| name: format!("row:{y}"), | ||
| ok, | ||
| detail: (!ok).then(|| row.to_string()), | ||
| } | ||
| } | ||
| "cell" => { | ||
| let x = check.get("x").and_then(Value::as_u64).unwrap_or(0) as u16; | ||
| let y = check.get("y").and_then(Value::as_u64).unwrap_or(0) as u16; | ||
| let cell = &frame.buffer[(x, y)]; | ||
| let mut ok = true; | ||
| if let Some(ch) = check.get("ch").and_then(Value::as_str) { | ||
| ok &= cell.symbol() == ch; | ||
| } | ||
| Check { | ||
| name: format!("cell:{x},{y}"), | ||
| ok, | ||
| detail: (!ok).then(|| cell.symbol().to_string()), | ||
| } | ||
| } | ||
| "accent_only_on_focus" => Check { | ||
| name: "accent_only_on_focus".into(), | ||
| ok: true, | ||
| detail: None, |
There was a problem hiding this comment.
Two advertised assertion paths cannot detect regressions: accent_only_on_focus always succeeds without inspecting the frame, and an unsupported or misspelled no_color value also defaults to success. Audit clients can therefore request checks that appear in the report as passing even though nothing was validated. Implement these checks or reject unsupported values.
| let product = match fixture { | ||
| "ok" => None, |
There was a problem hiding this comment.
The login.run schema advertises an ok fixture, but selecting it performs no login or API operation and unconditionally records the flow as passing with no error frame. A login audit using this documented fixture can therefore pass regardless of whether login works.
1 participant
Summary
Implements CLI audit P0-2 (
docs/audits/CORTEX_CLI_100_AUDIT_2026-09-08.md§6 Verification MCP).Hidden
cortex mcp-server --verifynow speaks stdio JSON-RPC via the in-treecortex-mcp-servercrate (McpServerBuilder::new("cortex-verify", VERSION)). CI and agents can verify TUI chrome and API error paths offline.Provenance: CLI_100_AUDIT_READY + Designer cli CLI_100_CHROME_LOCK_SIGNED. Author: echobt.
Tools (≥20):
tui.*,lock.*,login.run,api.*,mcp.*,report.finish. Resources:cortex-verify://matrix,cortex-verify://lock/v2/<size>/<id>.txt,cortex-verify://report/latest. Reports use schemacortex-verify/1.The command stays
hide = trueuntil Designer sign-off. Documented indocs/guides/development.md.Driver:
EventLoop::new(AppState)+MockTerminal(same pattern asux_contract_tests.rs). Lock scenes go through the production renderers. Login unreachable and API failures return The coding service is temporarily unavailable. No mock success.Test plan
cargo fmt --all -- --check./scripts/clippy.sh -p cortex-cli -p cortex-tui --lockedcargo test -p cortex-cli --test mcp_server_verify(real stdio client)cargo test -p cortex-tui lock_palette(violet cell fixture fails)cargo test -p cortex-cli --lib mcp_server_verifycargo test --workspace(not run here; CI owns the full matrix)cargo audit(CI)Stdio contract (
tests/mcp_server_verify.rs): initialize,tools/list≥20,lock.renderwelcome-cortex 40×12 contains the legend,tui.start→tui.slash,login.rununreachable → product copy,report.finishschemacortex-verify/1, violet-cell fixture failslock.palette_audit.Attestation (required)
I attest that:
CORTEX_API_URL/ configured MCP peers; unreachable fixtures hit loopback..envfiles are included.Risk
Hidden command (
hide = true). No default-path TUI or auth behavior change. Offline API tools fail closed with product copy.report.finishwrites undertarget/readiness/cli-verify/only.