diff --git a/.gitignore b/.gitignore index f3692dba2b..5234f815eb 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,10 @@ captures/ /*.avif /*.mp4 /*.png +.personal/ + +# Orion-OS local working context (memory bank, agent config) — never committed. +.agents/ + +# macOS Finder metadata +.DS_Store diff --git a/README.md b/README.md index db18f389cc..983cfdb481 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,10 @@ irm https://jcode.sh/install.ps1 | iex Need Homebrew, source builds, provider setup, or want an agent to set it up for you? [Jump to detailed installation](#detailed-installation). +Installs and updates verify a SHA-256 checksum over HTTPS. For the full +supply-chain trust model (what is and isn't verified, and how to install more +cautiously), see [`SECURITY.md`](SECURITY.md). + --- @@ -306,6 +310,10 @@ Jcode is left-aligned by default. You can switch to centered mode with the `Alt+ To disable emoji globally in TUI and CLI output, set `emoji = false` under `[display]` in `~/.jcode/config.toml`, or launch with `JCODE_NO_EMOJI=1`. Jcode replaces emoji with compact ASCII markers while preserving other Unicode text. +### Accessibility + +jcode is keyboard-driven, honors `NO_COLOR`/`JCODE_NO_COLOR`, encodes status with glyphs (not color alone), and measures theme contrast. Because it is a terminal UI, it has no dedicated screen-reader announcement channel yet. See [`docs/ACCESSIBILITY.md`](docs/ACCESSIBILITY.md) for an honest account of what works today, current limitations, and recommendations for screen-reader users. + --- ## Swarm diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000000..5da1da3dec --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,72 @@ +# Security + +This document covers jcode's software supply-chain trust model for +installation and updates (audit finding SEC-07), and where to report issues. + +## Reporting a vulnerability + +Please report security issues privately to the maintainer rather than opening a +public issue. Include repro steps and the affected version/commit. + +## Install & update trust model (SEC-07) + +### What is verified today + +- **Checksum integrity.** The install script and the in-app updater download a + `SHA256SUMS` file and verify the downloaded binary's SHA-256 against it + (`scripts/install.sh`, `crates/jcode-update-core::verify_asset_checksum_text`). + The installer tries multiple checksum sources (the release metadata host and + the GitHub release assets) before trusting one. +- **Transport.** All downloads are over HTTPS/TLS. + +This reliably prevents **accidental corruption** (partial downloads, mirror +rot, CDN glitches). + +### What is NOT verified — and the residual risk + +- **No independent cryptographic signature.** The `SHA256SUMS` file and the + binary share the same trust root (the release host / repository). An attacker + who compromises that origin can serve a malicious binary *and* a matching + `SHA256SUMS`, and checksum verification will pass. Checksums prove integrity, + not authenticity. +- **`curl | bash` install is trust-on-first-use.** `curl -fsSL + https://jcode.sh/install | bash` (and the PowerShell equivalent) pipes remote + code into a shell with the user's privileges. This is industry-standard for + CLI tools but means a compromise of the install host serves code directly. +- **Prebuilt binaries are not code-signed/notarized** at time of writing, so the + OS cannot independently attest their origin. + +### Recommendations for cautious users + +- Prefer building from source (`cargo build --release`) if you want to avoid the + `curl | bash` trust-on-first-use step. +- Or download the release archive and the `SHA256SUMS` from the GitHub release + page, inspect the installer script before running it, and verify the checksum + by hand. +- Pin to a specific released version rather than always taking latest. + +### Hardening roadmap (tracked, not yet implemented) + +These are the concrete steps to close the authenticity gap. They are recorded +here so the trust model is honest and the work is visible; they intentionally +require a maintainer decision on signing-key custody and are out of scope for +the change that added this document. + +1. **Detached signatures over `SHA256SUMS`.** Sign the sums file with a + long-lived key (minisign/signify or cosign/Sigstore) whose public key is + published out-of-band (README, website, and a pinned repo file). Verify the + signature in `jcode-update-core` and `scripts/install.sh` before trusting any + checksum. `jcode-update-core` already isolates checksum parsing/verification, + so a signature-verification step slots in ahead of it behind an opt-in + configured public key (no behavior change until a key ships). +2. **Multi-channel checksum/signature publication.** Publish the sums (and their + signature) on at least two independent roots (e.g. GitHub releases + a signed + git tag) so a single-origin compromise is insufficient. +3. **Platform code signing / notarization.** Sign+notarize macOS binaries and + sign Windows binaries so the OS attests origin; verify update payloads + against those signatures where the platform supports it. +4. **Pin the installer artifact hash inside the install script** so the script + and the artifact it fetches are bound together. + +Until (1)-(3) land, treat installation and updates as trust-on-first-use rooted +in the release host's integrity. diff --git a/crates/jcode-app-core/src/network_retry.rs b/crates/jcode-app-core/src/network_retry.rs index 3c19ced1bf..ea56192342 100644 --- a/crates/jcode-app-core/src/network_retry.rs +++ b/crates/jcode-app-core/src/network_retry.rs @@ -97,13 +97,59 @@ pub fn wait_plan() -> NetworkWaitPlan { } } -pub async fn wait_until_probably_online() { +/// Default ceiling for a single reconnect wait. Long enough to ride out a VPN +/// flap or a sleeping laptop, short enough that a permanently offline state +/// (broken VPN profile, captive portal that never satisfies the probe) cannot +/// wedge a caller forever (REL-01). +pub const DEFAULT_RECONNECT_CEILING: Duration = Duration::from_secs(300); + +/// Outcome of a bounded reconnect wait. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ReconnectOutcome { + /// Connectivity was observed; the caller should retry its request. + Online, + /// The ceiling elapsed while still offline; the caller must surface this + /// and stop rather than block forever. + GaveUp { waited: Duration }, +} + +impl ReconnectOutcome { + pub fn is_online(self) -> bool { + matches!(self, ReconnectOutcome::Online) + } +} + +/// Wait until connectivity is probably restored, bounded by [`DEFAULT_RECONNECT_CEILING`]. +/// +/// REL-01: previously this looped forever with exponential backoff and no +/// ceiling, so a permanent offline state wedged the caller with no escape. It +/// now returns [`ReconnectOutcome::GaveUp`] once the ceiling elapses so callers +/// can fail with a visible diagnostic instead of hanging. +pub async fn wait_until_probably_online() -> ReconnectOutcome { + wait_until_probably_online_bounded(DEFAULT_RECONNECT_CEILING).await +} + +/// Bounded reconnect wait with an explicit total-time ceiling. +/// +/// Polls with exponential backoff (capped at 30s per interval) until either +/// connectivity is observed ([`ReconnectOutcome::Online`]) or `max_total` +/// elapses ([`ReconnectOutcome::GaveUp`]). A zero or negative budget still makes +/// at least one connectivity probe so a transient blip is caught cheaply. +pub async fn wait_until_probably_online_bounded(max_total: Duration) -> ReconnectOutcome { + let start = std::time::Instant::now(); let mut delay = Duration::from_secs(1); loop { if probe_connectivity().await { - return; + return ReconnectOutcome::Online; } - wait_for_platform_change_or_delay(delay).await; + let elapsed = start.elapsed(); + if elapsed >= max_total { + return ReconnectOutcome::GaveUp { waited: elapsed }; + } + // Never sleep past the remaining budget, so we return close to the + // ceiling rather than overshooting by a full backoff interval. + let remaining = max_total - elapsed; + wait_for_platform_change_or_delay(delay.min(remaining)).await; delay = (delay * 2).min(Duration::from_secs(30)); } } @@ -183,6 +229,36 @@ async fn wait_for_command_output(command: &str, args: &[&str]) { mod tests { use super::*; + /// REL-01: a bounded wait must return within roughly its ceiling and never + /// hang, whatever the network state. With a tiny budget it resolves fast; + /// if offline it reports `GaveUp` rather than looping forever. + #[tokio::test] + async fn bounded_wait_respects_its_ceiling() { + let ceiling = Duration::from_millis(200); + let start = std::time::Instant::now(); + let outcome = wait_until_probably_online_bounded(ceiling).await; + let elapsed = start.elapsed(); + + // Must not overshoot the ceiling by more than one probe timeout (5s) + // plus scheduling slack — the key property is that it terminates. + assert!( + elapsed < ceiling + Duration::from_secs(8), + "bounded wait ran {elapsed:?}, far past its {ceiling:?} ceiling" + ); + // Whatever the CI network state, the outcome must be one of the two + // terminal states (i.e. the function returned at all). + match outcome { + ReconnectOutcome::Online => assert!(outcome.is_online()), + ReconnectOutcome::GaveUp { waited } => { + assert!(!outcome.is_online()); + assert!( + waited >= ceiling, + "GaveUp waited {waited:?} < ceiling {ceiling:?}" + ); + } + } + } + #[test] fn classifies_common_network_errors() { assert!(classify_message("connection reset by peer").is_some()); diff --git a/crates/jcode-app-core/src/tool/bash.rs b/crates/jcode-app-core/src/tool/bash.rs index 6dbbe87722..af3a287578 100644 --- a/crates/jcode-app-core/src/tool/bash.rs +++ b/crates/jcode-app-core/src/tool/bash.rs @@ -835,9 +835,8 @@ fn default_true() -> bool { true } -#[path = "bash_destructive_gate.rs"] -mod destructive_gate; -use destructive_gate::destructive_command_refusal; +use super::destructive_gate; +use super::destructive_gate::destructive_command_refusal; #[async_trait] impl Tool for BashTool { fn name(&self) -> &str { diff --git a/crates/jcode-app-core/src/tool/bash_destructive_gate.rs b/crates/jcode-app-core/src/tool/bash_destructive_gate.rs deleted file mode 100644 index b9c0f551e1..0000000000 --- a/crates/jcode-app-core/src/tool/bash_destructive_gate.rs +++ /dev/null @@ -1,88 +0,0 @@ -//! The destructive-command gate for the `bash` tool (issue #604). -//! -//! Kept in its own file so the policy seam is easy to find and review: this is -//! the only thing standing between a model's `rm -rf` and the user's data. - -/// Apply the deterministic destructive-command gate, returning refusal text -/// when the command must not run as-issued. -/// -/// Stage 1 is a pure blast-radius assessment; stage 2 turns a `Confirm` verdict -/// into a reflection prompt that a blind retry cannot satisfy. Catastrophic -/// targets (`/`, `$HOME`, credential stores, device nodes) are denied outright. -/// See issue #604. -pub(super) fn destructive_command_refusal( - command: &str, - justification: Option<&str>, - working_dir: Option, -) -> Option { - let risk_ctx = jcode_command_risk::RiskContext::from_env(working_dir); - let assessment = jcode_command_risk::assess(command, &risk_ctx); - if assessment.level.runs_immediately() { - return None; - } - - let justification = jcode_command_risk::Justification { - text: justification.map(str::to_string), - }; - match jcode_command_risk::gate(&assessment, &justification) { - jcode_command_risk::GateOutcome::Allow => None, - jcode_command_risk::GateOutcome::Deny { reason } => { - crate::logging::warn(&format!("[bash] denied destructive command: {command}")); - Some(reason) - } - jcode_command_risk::GateOutcome::Reflect { prompt } => { - crate::logging::info(&format!( - "[bash] destructive command held for justification: {command}" - )); - Some(prompt) - } - } -} - -/// The `bash` tool's JSON schema, including the `justification` field the -/// destructive-command gate consumes. -/// -/// Lives beside the gate so the schema and the policy that reads it stay in -/// sync, and so bash.rs stays inside the code-size budget. -pub(super) fn bash_parameters_schema() -> serde_json::Value { - let cmd_desc = if cfg!(windows) { - "The Windows command to execute via cmd.exe. Use cmd.exe syntax and quoting, not Bash syntax." - } else { - "The bash command to execute. Put large temp files under `$JCODE_SCRATCH_DIR`, not `/tmp`." - }; - serde_json::json!({ - "type": "object", - "required": ["command"], - "properties": { - "intent": crate::tool::intent_schema_property(), - "command": { - "type": "string", - "description": cmd_desc - }, - "timeout": { - "type": "integer", - "description": "Timeout in MILLISECONDS (not seconds), e.g. 600000 = 10min; kills with exit 124. Omit for no timeout." - }, - "run_in_background": { - "type": "boolean", - "description": "Run in background. Emit `JCODE_PROGRESS {json}` lines for progress reporting." - }, - "notify": { - "type": "boolean", - "description": "Notify on completion." - }, - "wake": { - "type": "boolean", - "description": "Wake on completion." - }, - "stall_wake_seconds": { - "type": "integer", - "description": "With run_in_background: wake the agent after this many seconds of no output/progress (min 30, resets on activity). Use for long jobs that may hang silently." - }, - "justification": { - "type": "string", - "description": "Only when re-issuing a command the destructive gate refused; explain which user request it serves." - } - } - }) -} diff --git a/crates/jcode-app-core/src/tool/computer/mod.rs b/crates/jcode-app-core/src/tool/computer/mod.rs index 409a62661c..b3b40182cc 100644 --- a/crates/jcode-app-core/src/tool/computer/mod.rs +++ b/crates/jcode-app-core/src/tool/computer/mod.rs @@ -122,6 +122,10 @@ struct ComputerInput { /// For mutating actions: resolve and report the target without acting. #[serde(default)] dry_run: Option, + /// Only when re-issuing a `run_applescript`/`run_jxa` script the destructive + /// gate refused (SEC-02): explain which user request it serves. + #[serde(default)] + justification: Option, } /// Cap a tool output's text so a huge AX tree / clipboard / OCR dump cannot @@ -245,27 +249,29 @@ impl Tool for ComputerTool { "timeout_ms": { "type": "integer" }, "region": { "type": "array", "items": { "type": "number" }, "description": "ocr region [x,y,w,h]; omit for full screen." }, "level": { "type": "number", "description": "set_brightness 0..1." }, - "dry_run": { "type": "boolean", "description": "Mutating actions: report intended action without doing it." } + "dry_run": { "type": "boolean", "description": "Mutating actions: report intended action without doing it." }, + "justification": { "type": "string", "description": "Only when re-issuing a run_applescript/run_jxa script the destructive gate refused: explain which user request it serves." } } }) } - async fn execute(&self, input: Value, _ctx: ToolContext) -> Result { + async fn execute(&self, input: Value, ctx: ToolContext) -> Result { let parsed: ComputerInput = serde_json::from_value(input).context("invalid `macos_computer_use` tool input")?; - tokio::task::spawn_blocking(move || run(parsed)) + let working_dir = ctx.working_dir.clone(); + tokio::task::spawn_blocking(move || run(parsed, working_dir)) .await .context("macos_computer_use tool task panicked")? } } #[cfg(not(target_os = "macos"))] -fn run(_input: ComputerInput) -> Result { +fn run(_input: ComputerInput, _working_dir: Option) -> Result { bail!("The `macos_computer_use` tool is only supported on macOS.") } #[cfg(target_os = "macos")] -fn run(input: ComputerInput) -> Result { +fn run(input: ComputerInput, working_dir: Option) -> Result { let action = input.action.as_str(); // dry_run: for mutating actions, report the intended target and stop. @@ -276,13 +282,17 @@ fn run(input: ComputerInput) -> Result { ))); } - let result = dispatch(action, &input); + let result = dispatch(action, &input, working_dir); // Cap large textual outputs to protect context (images are unaffected). result.map(|o| cap_output(o, 16_000)) } #[cfg(target_os = "macos")] -fn dispatch(action: &str, input: &ComputerInput) -> Result { +fn dispatch( + action: &str, + input: &ComputerInput, + working_dir: Option, +) -> Result { match action { // ---- discovery & setup ---- "discover" => discover::discover(input.category.as_deref()), @@ -463,6 +473,14 @@ fn dispatch(action: &str, input: &ComputerInput) -> Result { .script .as_deref() .context("run_applescript requires `script`")?; + // SEC-02: apply the shipped #604 destructive gate to scripting. + if let Some(refusal) = super::destructive_gate::applescript_destructive_refusal( + s, + input.justification.as_deref(), + working_dir, + ) { + bail!(refusal); + } sys::run_applescript(s) } "run_jxa" => { @@ -470,6 +488,14 @@ fn dispatch(action: &str, input: &ComputerInput) -> Result { .script .as_deref() .context("run_jxa requires `script`")?; + // SEC-02: apply the shipped #604 destructive gate to scripting. + if let Some(refusal) = super::destructive_gate::applescript_destructive_refusal( + s, + input.justification.as_deref(), + working_dir, + ) { + bail!(refusal); + } sys::run_jxa(s) } "wait_for" => { diff --git a/crates/jcode-app-core/src/tool/destructive_gate.rs b/crates/jcode-app-core/src/tool/destructive_gate.rs new file mode 100644 index 0000000000..e34f95abd6 --- /dev/null +++ b/crates/jcode-app-core/src/tool/destructive_gate.rs @@ -0,0 +1,401 @@ +//! The destructive-command gate for the `bash` tool (issue #604). +//! +//! Kept in its own file so the policy seam is easy to find and review: this is +//! the only thing standing between a model's `rm -rf` and the user's data. + +/// Apply the deterministic destructive-command gate, returning refusal text +/// when the command must not run as-issued. +/// +/// Stage 1 is a pure blast-radius assessment; stage 2 turns a `Confirm` verdict +/// into a reflection prompt that a blind retry cannot satisfy. Catastrophic +/// targets (`/`, `$HOME`, credential stores, device nodes) are denied outright. +/// See issue #604. +pub(crate) fn destructive_command_refusal( + command: &str, + justification: Option<&str>, + working_dir: Option, +) -> Option { + destructive_command_refusal_labeled(command, justification, working_dir, "bash") +} + +/// Same policy as [`destructive_command_refusal`], but with a caller label so +/// log lines identify which tool surface issued the command (e.g. `bash`, +/// `macos_computer_use`). Reused by the computer-use scripting gate (SEC-02) so +/// AppleScript/JXA go through the exact same shipped, tested #604 policy. +pub(crate) fn destructive_command_refusal_labeled( + command: &str, + justification: Option<&str>, + working_dir: Option, + caller: &str, +) -> Option { + let risk_ctx = jcode_command_risk::RiskContext::from_env(working_dir); + let assessment = jcode_command_risk::assess(command, &risk_ctx); + if assessment.level.runs_immediately() { + return None; + } + + let justification = jcode_command_risk::Justification { + text: justification.map(str::to_string), + }; + match jcode_command_risk::gate(&assessment, &justification) { + jcode_command_risk::GateOutcome::Allow => None, + jcode_command_risk::GateOutcome::Deny { reason } => { + crate::logging::warn(&format!("[{caller}] denied destructive command: {command}")); + Some(reason) + } + jcode_command_risk::GateOutcome::Reflect { prompt } => { + crate::logging::info(&format!( + "[{caller}] destructive command held for justification: {command}" + )); + Some(prompt) + } + } +} + +/// The `bash` tool's JSON schema, including the `justification` field the +/// destructive-command gate consumes. +/// +/// Lives beside the gate so the schema and the policy that reads it stay in +/// sync, and so bash.rs stays inside the code-size budget. +pub(crate) fn bash_parameters_schema() -> serde_json::Value { + let cmd_desc = if cfg!(windows) { + "The Windows command to execute via cmd.exe. Use cmd.exe syntax and quoting, not Bash syntax." + } else { + "The bash command to execute. Put large temp files under `$JCODE_SCRATCH_DIR`, not `/tmp`." + }; + serde_json::json!({ + "type": "object", + "required": ["command"], + "properties": { + "intent": crate::tool::intent_schema_property(), + "command": { + "type": "string", + "description": cmd_desc + }, + "timeout": { + "type": "integer", + "description": "Timeout in MILLISECONDS (not seconds), e.g. 600000 = 10min; kills with exit 124. Omit for no timeout." + }, + "run_in_background": { + "type": "boolean", + "description": "Run in background. Emit `JCODE_PROGRESS {json}` lines for progress reporting." + }, + "notify": { + "type": "boolean", + "description": "Notify on completion." + }, + "wake": { + "type": "boolean", + "description": "Wake on completion." + }, + "stall_wake_seconds": { + "type": "integer", + "description": "With run_in_background: wake the agent after this many seconds of no output/progress (min 30, resets on activity). Use for long jobs that may hang silently." + }, + "justification": { + "type": "string", + "description": "Only when re-issuing a command the destructive gate refused; explain which user request it serves." + } + } + }) +} + +// ============================================================================ +// SEC-02: destructive-command gate for macOS computer-use scripting. +// +// `macos_computer_use`'s `run_applescript` / `run_jxa` actions execute +// model-supplied source via `/usr/bin/osascript`. AppleScript can shell out +// with `do shell script "..."` and JXA with `Application(...).doShellScript(...)` +// / `$.NSTask` / `ObjC` bridges, so an ungated script is a #604-class data +// destruction path that neighbours the `bash` tool while bypassing its gate. +// +// This is defense-in-depth, honestly scoped (cf. SEC-05): a static parser +// cannot catch every obfuscation an interpreter allows. It reuses the shipped, +// tested #604 policy for the common, high-signal cases: embedded `do shell +// script` / `doShellScript` payloads are routed through the exact same gate as +// bash, and a small set of native permanent-destruction verbs are treated as +// requiring justification. +// ============================================================================ + +/// Return refusal text when an AppleScript/JXA `script` must not run as issued. +/// +/// `None` means "no destructive signal detected, proceed"; `Some(reason)` is a +/// refusal/reflection prompt to surface to the model, mirroring the bash gate. +pub(crate) fn applescript_destructive_refusal( + script: &str, + justification: Option<&str>, + working_dir: Option, +) -> Option { + // 1) Handle embedded shell payloads. + for payload in extract_embedded_shell_commands(script) { + match payload { + // A readable literal: route through the exact shipped #604 gate. + ShellPayload::Literal(cmd) => { + if let Some(refusal) = destructive_command_refusal_labeled( + &cmd, + justification, + working_dir.clone(), + "macos_computer_use", + ) { + return Some(refusal); + } + } + // A computed/opaque argument we cannot inspect statically. We cannot + // prove it is safe, so hold it for justification (reflection-level), + // mirroring how the #604 gate treats an unknown-target command. + ShellPayload::Dynamic => { + if justification + .map(str::trim) + .filter(|s| !s.is_empty()) + .is_none() + { + crate::logging::warn( + "[macos_computer_use] held script: `do shell script` with a \ + non-literal (computed) argument", + ); + return Some( + "This script passes a computed value to `do shell script`, so its \ + effect cannot be verified before it runs. Because the scripting \ + actions bypass the terminal's destructive-command gate (#604), it \ + is held.\n\nRe-issue with a `justification` explaining which user \ + request it serves, or inline the exact shell command as a string \ + literal so it can be checked." + .to_string(), + ); + } + crate::logging::info( + "[macos_computer_use] allowed dynamic `do shell script` with justification", + ); + } + } + } + + // 2) Flag native permanent-destruction verbs the shell gate cannot see. + if let Some(verb) = detect_native_destruction_verb(script) { + if justification + .map(str::trim) + .filter(|s| !s.is_empty()) + .is_none() + { + crate::logging::warn(&format!( + "[macos_computer_use] held potentially destructive script (matched `{verb}`)" + )); + return Some(format!( + "This AppleScript/JXA uses `{verb}`, which can permanently delete or overwrite user data outside any recycle bin. The `macos_computer_use` scripting actions bypass the terminal, so this is held for the same reason the bash destructive-command gate (#604) holds `rm -rf`.\n\n If this is genuinely required by the user's request, re-issue the action with a `justification` explaining which request it serves and why a non-destructive approach will not work. Prefer moving items to the Trash (Finder `delete`) over `NSFileManager removeItem` / `rm`." + )); + } + crate::logging::info(&format!( + "[macos_computer_use] allowed script matching `{verb}` with justification" + )); + } + + None +} + +/// A shell payload discovered inside an AppleScript/JXA source. +enum ShellPayload { + /// A shell command we could read as a string literal — check it directly. + Literal(String), + /// A `do shell script` whose argument is a computed value we cannot read + /// statically (variable, concatenation, function call). Cannot be proven + /// safe, so it is held for justification rather than passed to the gate. + Dynamic, +} + +/// Extract shell payloads embedded in an AppleScript/JXA source via the +/// documented shell-escape verbs (`do shell script`, JXA `doShellScript`). +/// When a verb's argument is a static string literal we return it verbatim for +/// the #604 gate; when it is a computed value we return [`ShellPayload::Dynamic`] +/// so the caller can hold it rather than silently allow it. +fn extract_embedded_shell_commands(script: &str) -> Vec { + let lower = script.to_ascii_lowercase(); + // Verbs that hand a string to a shell. Keep this list tight and documented. + const VERBS: [&str; 3] = ["do shell script", "doshellscript", "shellscript"]; + + let mut out = Vec::new(); + for verb in VERBS { + let mut from = 0usize; + while let Some(rel) = lower[from..].find(verb) { + let idx = from + rel + verb.len(); + from = idx; + // Only accept a literal that begins at the verb's argument position + // (allowing whitespace / an opening paren for the JXA form). A literal + // that appears only *after* other tokens means the argument itself is + // computed, so we escalate as Dynamic. + match literal_at_argument_start(&script[idx..]) { + Some(cmd) => out.push(ShellPayload::Literal(cmd)), + None => out.push(ShellPayload::Dynamic), + } + } + } + out +} + +/// Read a quoted string literal that begins at the argument position following a +/// shell verb — i.e. after only insignificant tokens (whitespace, `(`, `:`). +/// Returns `None` if the argument is not an immediate literal (e.g. a variable), +/// which the caller treats as a non-inspectable dynamic argument. +fn literal_at_argument_start(s: &str) -> Option { + let mut chars = s.char_indices(); + for (i, c) in chars.by_ref() { + match c { + // Insignificant leading tokens between the verb and its argument. + ' ' | '\t' | '\n' | '\r' | '(' | ':' => continue, + '"' | '\'' => return first_string_literal_after(&s[i..]), + // Any other token first means the argument is not an immediate + // literal (variable name, expression, etc.). + _ => return None, + } + } + None +} + +/// Read the first double- or single-quoted string literal in `s`, unescaping +/// the common `\"` / `\'` sequences. Returns `None` if no literal is found. +fn first_string_literal_after(s: &str) -> Option { + let bytes = s.as_bytes(); + let mut i = 0; + while i < bytes.len() { + let c = bytes[i] as char; + if c == '"' || c == '\'' { + let quote = c; + let mut out = String::new(); + let mut j = i + 1; + while j < bytes.len() { + let cj = bytes[j] as char; + if cj == '\\' && j + 1 < bytes.len() { + out.push(bytes[j + 1] as char); + j += 2; + continue; + } + if cj == quote { + return Some(out); + } + out.push(cj); + j += 1; + } + return Some(out); // unterminated literal: return what we have + } + i += 1; + } + None +} + +/// Detect native (non-shell) permanent-destruction verbs. Returns the matched +/// token for use in the refusal message. Deliberately narrow: Finder `delete` +/// moves to Trash and is intentionally NOT matched. +fn detect_native_destruction_verb(script: &str) -> Option<&'static str> { + let lower = script.to_ascii_lowercase(); + const VERBS: [&str; 5] = [ + "removeitematpath", // NSFileManager removeItemAtPath: + "removeitematurl", // NSFileManager removeItemAtURL: + "nstask", // arbitrary process launch (shell-equivalent) + "trashitematurl", // ok to Trash, but pair with recursive removal below + "removeitem", // generic NSFileManager removeItem + ]; + for v in VERBS { + // `trashitematurl` alone is non-destructive (recycle bin); only flag it + // when combined with an explicit unlink verb elsewhere in the script. + if v == "trashitematurl" { + continue; + } + if lower.contains(v) { + return Some(match v { + "removeitematpath" => "NSFileManager removeItemAtPath", + "removeitematurl" => "NSFileManager removeItemAtURL", + "removeitem" => "NSFileManager removeItem", + "nstask" => "NSTask", + _ => "destructive file API", + }); + } + } + None +} + +#[cfg(test)] +mod sec02_scripting_gate_tests { + //! SEC-02: `macos_computer_use` scripting (`run_applescript`/`run_jxa`) must + //! route embedded shell payloads through the shipped #604 gate and flag + //! native permanent-destruction verbs. These tests are pure logic (no + //! osascript), so they run on any platform. + use super::*; + + #[test] + fn allows_benign_shell_script() { + assert!( + applescript_destructive_refusal("do shell script \"echo hi\"", None, None).is_none(), + "a harmless echo must not be gated" + ); + } + + #[test] + fn allows_non_destructive_applescript() { + // Pure GUI automation with no shell/removal verbs proceeds. + let script = "tell application \"Finder\" to activate"; + assert!(applescript_destructive_refusal(script, None, None).is_none()); + } + + #[test] + fn blocks_catastrophic_embedded_shell_even_with_justification() { + // `rm -rf $HOME` is catastrophic in the #604 policy: no justification + // can unlock it, and it must be caught when embedded in AppleScript. + let script = "do shell script \"rm -rf $HOME\""; + assert!(applescript_destructive_refusal(script, None, None).is_some()); + assert!( + applescript_destructive_refusal(script, Some("user asked"), None).is_some(), + "catastrophic targets stay blocked regardless of justification" + ); + } + + #[test] + fn flags_native_removeitem_without_justification() { + // JXA file deletion via NSFileManager bypasses the shell gate; the + // native-verb detector must hold it for justification. + let script = + "$.NSFileManager.defaultManager.removeItemAtPathError('/Users/x/.ssh/id_ed25519')"; + let refusal = applescript_destructive_refusal(script, None, None) + .expect("removeItemAtPath must be gated"); + assert!(refusal.contains("removeItemAtPath")); + } + + #[test] + fn native_verb_passes_with_justification() { + // Unlike catastrophic shell targets, a native-verb match is a + // reflection-level hold: an explicit justification lets it proceed. + let script = "$.NSFileManager.defaultManager.removeItemAtPathError('/tmp/scratch')"; + assert!(applescript_destructive_refusal(script, None, None).is_some()); + assert!( + applescript_destructive_refusal(script, Some("clean up my temp scratch dir"), None) + .is_none(), + "a justified native removal should proceed" + ); + } + + #[test] + fn detects_do_shell_script_case_insensitively() { + // AppleScript is case-insensitive; the verb scan must be too. + let script = "DO SHELL SCRIPT \"rm -rf $HOME\""; + assert!(applescript_destructive_refusal(script, None, None).is_some()); + } + + #[test] + fn dynamic_shell_argument_is_escalated_not_ignored() { + // When `do shell script` receives a computed value we cannot read + // statically, we must still escalate (unknown target), never silently + // allow it. + let script = "set cmd to \"rm -rf \" & targetDir\ndo shell script cmd"; + assert!( + applescript_destructive_refusal(script, None, None).is_some(), + "a non-literal shell argument must be escalated, not ignored" + ); + } + + #[test] + fn first_string_literal_handles_escaped_quotes() { + // Sanity-check the literal extractor used by the shell-payload scan. + assert_eq!( + first_string_literal_after(" \"echo \\\"hi\\\"\""), + Some("echo \"hi\"".to_string()) + ); + } +} diff --git a/crates/jcode-app-core/src/tool/mod.rs b/crates/jcode-app-core/src/tool/mod.rs index fd5dc4eb0d..5c39d4cdcd 100644 --- a/crates/jcode-app-core/src/tool/mod.rs +++ b/crates/jcode-app-core/src/tool/mod.rs @@ -11,6 +11,8 @@ mod computer; mod config_edit_notice; mod conversation_search; mod debug_socket; +/// Shared #604 destructive-command gate (used by `bash` and `computer` scripting). +mod destructive_gate; mod discover; mod discover_secrets; mod edit; @@ -33,6 +35,8 @@ mod session_search; pub(crate) mod session_search_index; mod side_panel; mod skill; +/// SEC-03: shared SSRF destination guard for webfetch/websearch. +mod ssrf; mod todo; mod webfetch; mod websearch; diff --git a/crates/jcode-app-core/src/tool/ssrf.rs b/crates/jcode-app-core/src/tool/ssrf.rs new file mode 100644 index 0000000000..2759de30ed --- /dev/null +++ b/crates/jcode-app-core/src/tool/ssrf.rs @@ -0,0 +1,256 @@ +//! SEC-03: SSRF guard for agent-driven HTTP tools (`webfetch`, `websearch`). +//! +//! An AI agent that can be steered to fetch arbitrary URLs is a Server-Side +//! Request Forgery vector: it can be pointed at the host's own loopback +//! services, the LAN, or cloud instance-metadata endpoints +//! (`169.254.169.254`) to read credentials. This module resolves a URL's host +//! and refuses any destination that resolves to a non-public IP. +//! +//! DNS-rebinding hardening: we re-check every resolved address and reject if +//! ANY is private (defeats the "one public + one private A record" trick), and +//! the caller pins the connection to the validated address via +//! [`guard_public_url_pinned`] + reqwest `resolve()`, so the IP we checked is +//! the IP actually connected to — closing the resolve-then-connect TOCTOU gap +//! for the common case. Residual limits (cf. SEC-05): a hostile custom DNS that +//! returns different sets per lookup is bounded by pinning, but proxies and +//! any code path that bypasses the pinned client are not covered. There is no +//! allowlist escape hatch yet for legitimately-internal hosts. + +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; + +/// Validate that `raw_url` targets a public host, resolving DNS and checking +/// every returned address. Returns `Ok(())` when safe, or `Err` with a clear, +/// user-facing refusal naming the blocked class. +pub(crate) async fn guard_public_url(raw_url: &str) -> anyhow::Result<()> { + guard_public_url_pinned(raw_url).await.map(|_| ()) +} + +/// What a passing SSRF check resolved to, so the caller can *pin* the +/// connection to the exact validated address (closing the TOCTOU/DNS-rebinding +/// gap: reqwest reuses this address instead of re-resolving at connect time). +pub(crate) struct GuardedTarget { + /// The hostname to pin (only set when DNS was used, not for literal IPs). + pub host: Option, + /// A validated socket address to pin the host to. `None` for a literal-IP + /// URL, which needs no pinning because there is no name to re-resolve. + pub pinned: Option, +} + +/// Like [`guard_public_url`] but returns a [`GuardedTarget`] so the caller can +/// pin the connection to a validated IP. +pub(crate) async fn guard_public_url_pinned(raw_url: &str) -> anyhow::Result { + let url = url::Url::parse(raw_url) + .map_err(|e| anyhow::anyhow!("Could not parse URL for safety check: {e}"))?; + + match url.scheme() { + "http" | "https" => {} + other => anyhow::bail!("URL scheme `{other}` is not allowed; use http or https."), + } + + let host = url + .host_str() + .ok_or_else(|| anyhow::anyhow!("URL has no host to validate."))?; + + // A bracketed/literal IP host is checked directly (no DNS, no pinning + // needed — there is no name that could be re-resolved to something else). + if let Ok(ip) = host.parse::() { + reject_if_blocked(host, ip)?; + return Ok(GuardedTarget { + host: None, + pinned: None, + }); + } + + // Guard against obviously-internal names even if resolution is skipped by a + // proxy later (defense in depth; the resolve below is the real check). + let lower = host.to_ascii_lowercase(); + if lower == "localhost" || lower.ends_with(".localhost") || lower.ends_with(".local") { + anyhow::bail!( + "Refusing to fetch `{host}`: internal/loopback hostnames are blocked to prevent \ + server-side request forgery (SSRF)." + ); + } + + // Resolve. `lookup_host` needs a port; the scheme default is fine since we + // only care about the IP. + let port = url.port_or_known_default().unwrap_or(443); + let resolved: Vec = tokio::net::lookup_host((host, port)) + .await + .map_err(|e| anyhow::anyhow!("Could not resolve `{host}` for safety check: {e}"))? + .collect(); + + if resolved.is_empty() { + anyhow::bail!("`{host}` did not resolve to any address."); + } + // Reject if ANY resolved address is non-public (defeats "one public + one + // private A record" rebinding). + for addr in &resolved { + reject_if_blocked(host, addr.ip())?; + } + // Pin the connection to the first validated address so the IP we checked is + // the IP actually connected to, closing the resolve-then-connect TOCTOU gap. + Ok(GuardedTarget { + host: Some(host.to_string()), + pinned: resolved.into_iter().next(), + }) +} + +/// Reject a single resolved address if it is not a public, routable unicast IP. +fn reject_if_blocked(host: &str, ip: IpAddr) -> anyhow::Result<()> { + if let Some(reason) = blocked_reason(ip) { + anyhow::bail!( + "Refusing to fetch `{host}` ({ip}): {reason}. This is blocked to prevent \ + server-side request forgery (SSRF) against internal services and cloud \ + metadata. If you truly need an internal host, fetch it outside the agent." + ); + } + Ok(()) +} + +/// Why an IP is not a safe public destination, or `None` if it is fine. +fn blocked_reason(ip: IpAddr) -> Option<&'static str> { + match ip { + IpAddr::V4(v4) => blocked_reason_v4(v4), + IpAddr::V6(v6) => blocked_reason_v6(v6), + } +} + +fn blocked_reason_v4(ip: Ipv4Addr) -> Option<&'static str> { + if ip.is_loopback() { + return Some("loopback address"); + } + if ip.is_private() { + return Some("private network address"); + } + if ip.is_link_local() { + // Covers 169.254.0.0/16, including the 169.254.169.254 metadata IP. + return Some("link-local address (includes cloud metadata 169.254.169.254)"); + } + if ip.is_unspecified() { + return Some("unspecified address 0.0.0.0"); + } + if ip.is_broadcast() { + return Some("broadcast address"); + } + if ip.is_multicast() { + return Some("multicast address"); + } + // Carrier-grade NAT 100.64.0.0/10 (is_shared is unstable in std; check by hand). + let o = ip.octets(); + if o[0] == 100 && (64..=127).contains(&o[1]) { + return Some("carrier-grade NAT address"); + } + // 192.0.0.0/24 IETF protocol assignments, 198.18.0.0/15 benchmarking. + if o[0] == 192 && o[1] == 0 && o[2] == 0 { + return Some("IETF-reserved address"); + } + if o[0] == 198 && (o[1] == 18 || o[1] == 19) { + return Some("benchmarking-reserved address"); + } + None +} + +fn blocked_reason_v6(ip: Ipv6Addr) -> Option<&'static str> { + if ip.is_loopback() { + return Some("IPv6 loopback address"); + } + if ip.is_unspecified() { + return Some("IPv6 unspecified address ::"); + } + if ip.is_multicast() { + return Some("IPv6 multicast address"); + } + let seg = ip.segments(); + // Unique local addresses fc00::/7. + if (seg[0] & 0xfe00) == 0xfc00 { + return Some("IPv6 unique-local address"); + } + // Link-local fe80::/10. + if (seg[0] & 0xffc0) == 0xfe80 { + return Some("IPv6 link-local address"); + } + // IPv4-mapped ::ffff:0:0/96 — unwrap and apply the v4 rules so a mapped + // 127.0.0.1 / 169.254.x cannot slip through. + if let Some(v4) = ip.to_ipv4_mapped() { + return blocked_reason_v4(v4); + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + fn blocked(ip: &str) -> bool { + blocked_reason(ip.parse().unwrap()).is_some() + } + + #[test] + fn blocks_loopback_private_and_metadata() { + assert!(blocked("127.0.0.1")); + assert!(blocked("10.0.0.5")); + assert!(blocked("192.168.1.1")); + assert!(blocked("172.16.0.1")); + assert!(blocked("169.254.169.254"), "cloud metadata must be blocked"); + assert!(blocked("0.0.0.0")); + assert!(blocked("100.64.0.1"), "CGNAT must be blocked"); + assert!(blocked("::1")); + assert!(blocked("fd00::1"), "IPv6 ULA must be blocked"); + assert!(blocked("fe80::1"), "IPv6 link-local must be blocked"); + assert!( + blocked("::ffff:127.0.0.1"), + "mapped loopback must be blocked" + ); + assert!( + blocked("::ffff:169.254.169.254"), + "mapped metadata must be blocked" + ); + } + + #[test] + fn allows_public_addresses() { + assert!(!blocked("8.8.8.8")); + assert!(!blocked("1.1.1.1")); + assert!(!blocked("93.184.216.34")); // example.com + assert!(!blocked("2606:4700:4700::1111")); // cloudflare v6 + } + + #[tokio::test] + async fn guard_rejects_literal_internal_urls() { + assert!(guard_public_url("http://127.0.0.1/").await.is_err()); + assert!( + guard_public_url("http://169.254.169.254/latest/meta-data/") + .await + .is_err() + ); + assert!(guard_public_url("http://[::1]:6379/").await.is_err()); + assert!( + guard_public_url("http://localhost:8080/admin") + .await + .is_err() + ); + assert!( + guard_public_url("ftp://example.com/").await.is_err(), + "non-http scheme rejected" + ); + } + + #[tokio::test] + async fn pinned_guard_rejects_internal_and_needs_no_pin_for_literal_public_ip() { + // Internal targets are rejected by the pinned variant too. + assert!( + guard_public_url_pinned("http://169.254.169.254/") + .await + .is_err() + ); + assert!(guard_public_url_pinned("http://10.0.0.1/").await.is_err()); + + // A literal public IP passes and needs no pinning (there is no name to + // re-resolve), so `host`/`pinned` are None. + let t = guard_public_url_pinned("http://1.1.1.1/") + .await + .expect("public literal IP should pass"); + assert!(t.host.is_none(), "literal-IP URL needs no host pin"); + assert!(t.pinned.is_none(), "literal-IP URL needs no pinned address"); + } +} diff --git a/crates/jcode-app-core/src/tool/webfetch.rs b/crates/jcode-app-core/src/tool/webfetch.rs index fb57592fc0..61f85aba42 100644 --- a/crates/jcode-app-core/src/tool/webfetch.rs +++ b/crates/jcode-app-core/src/tool/webfetch.rs @@ -21,11 +21,44 @@ pub struct WebFetchTool { client: reqwest::Client, } +/// Max redirects webfetch will follow manually. Each hop is re-validated by the +/// SSRF guard, so a public URL cannot bounce the fetch to an internal one. +const MAX_REDIRECTS: usize = 5; + impl WebFetchTool { pub fn new() -> Self { - Self { - client: crate::provider::shared_http_client(), - } + // SEC-03: webfetch does NOT auto-follow redirects. reqwest's default + // policy would silently chase a 30x into loopback/metadata after our + // pre-flight guard already passed. We follow manually and re-guard every + // hop (see `execute`). Falls back to the shared client if the dedicated + // build fails, so webfetch never becomes unavailable. + let client = reqwest::Client::builder() + .user_agent("Mozilla/5.0 (compatible; JCode/1.0)") + .redirect(reqwest::redirect::Policy::none()) + .build() + .unwrap_or_else(|_| crate::provider::shared_http_client()); + Self { client } + } + + /// Build a no-redirect client that pins the guarded host to the exact IP the + /// SSRF guard validated (SEC-03 DNS-rebinding hardening). Returns `None` + /// when there is nothing to pin (literal-IP URL), so the caller uses the + /// default client. `resolve()` overrides DNS for this host only, so reqwest + /// connects to the checked address instead of re-resolving. + fn pinned_client(&self, target: &super::ssrf::GuardedTarget) -> Result { + let host = target + .host + .as_deref() + .ok_or_else(|| anyhow::anyhow!("DNS guard returned no host to pin"))?; + let addr = target + .pinned + .ok_or_else(|| anyhow::anyhow!("DNS guard returned no address to pin"))?; + reqwest::Client::builder() + .user_agent("Mozilla/5.0 (compatible; JCode/1.0)") + .redirect(reqwest::redirect::Policy::none()) + .resolve(host, addr) + .build() + .map_err(|e| anyhow::anyhow!("Failed to build pinned SSRF-safe HTTP client: {e}")) } } @@ -78,25 +111,67 @@ impl Tool for WebFetchTool { if !params.url.starts_with("http://") && !params.url.starts_with("https://") { return Err(anyhow::anyhow!("URL must start with http:// or https://")); } - let timeout = params.timeout.unwrap_or(DEFAULT_TIMEOUT).min(MAX_TIMEOUT); let format = params.format.as_deref().unwrap_or("markdown"); - let response = self - .client - .get(¶ms.url) - .header( - reqwest::header::USER_AGENT, - "Mozilla/5.0 (compatible; JCode/1.0)", - ) - .timeout(Duration::from_secs(timeout)) - .send() - .await?; + // SEC-03: follow redirects manually, re-validating EVERY hop against the + // SSRF guard so a public URL cannot 30x-redirect into loopback/private/ + // metadata space (the pre-flight check alone would miss that). For each + // hop we PIN the connection to the exact validated IP (reqwest + // `resolve()`), so the address we checked is the address connected to — + // closing the resolve-then-connect DNS-rebinding TOCTOU gap. + let mut current_url = params.url.clone(); + let mut redirects = 0usize; + let response = loop { + let target = super::ssrf::guard_public_url_pinned(¤t_url).await?; + let client = if target.host.is_some() { + // A DNS-resolved target must use the exact address that passed + // the SSRF check. Never fall back to an unpinned client: that + // would reintroduce the resolve/connect TOCTOU gap on a client + // construction failure. + self.pinned_client(&target)? + } else { + self.client.clone() + }; + let resp = client + .get(¤t_url) + .header( + reqwest::header::USER_AGENT, + "Mozilla/5.0 (compatible; JCode/1.0)", + ) + .timeout(Duration::from_secs(timeout)) + .send() + .await?; + + let status = resp.status(); + if status.is_redirection() { + let location = resp + .headers() + .get(reqwest::header::LOCATION) + .and_then(|v| v.to_str().ok()) + .ok_or_else(|| { + anyhow::anyhow!("HTTP {} redirect without a Location header", status) + })?; + // Resolve relative redirects against the current URL. + let next = reqwest::Url::parse(¤t_url) + .and_then(|base| base.join(location)) + .map_err(|e| anyhow::anyhow!("Invalid redirect target `{location}`: {e}"))?; + redirects += 1; + if redirects > MAX_REDIRECTS { + return Err(anyhow::anyhow!( + "Too many redirects (>{MAX_REDIRECTS}) starting from {}", + params.url + )); + } + current_url = next.to_string(); + continue; + } - let status = response.status(); - if !status.is_success() { - return Err(anyhow::anyhow!("HTTP error: {}", status)); - } + if !status.is_success() { + return Err(anyhow::anyhow!("HTTP error: {}", status)); + } + break resp; + }; // Check content length if let Some(len) = response.content_length() diff --git a/crates/jcode-base/Cargo.toml b/crates/jcode-base/Cargo.toml index 128247ee4f..a65aff6052 100644 --- a/crates/jcode-base/Cargo.toml +++ b/crates/jcode-base/Cargo.toml @@ -142,7 +142,7 @@ embeddings = ["dep:jcode-embedding"] bedrock = ["jcode-provider-bedrock/aws-sdk"] [target.'cfg(windows)'.dependencies] -windows-sys = { version = "0.59", features = ["Win32_Foundation", "Win32_System_Power", "Win32_System_Threading"] } +windows-sys = { version = "0.59", features = ["Win32_Foundation", "Win32_Storage_FileSystem", "Win32_System_IO", "Win32_System_Power", "Win32_System_Threading"] } [target.'cfg(target_os = "macos")'.dependencies] global-hotkey = "0.7" diff --git a/crates/jcode-base/src/auth/oauth.rs b/crates/jcode-base/src/auth/oauth.rs index 79e5e03444..b63bf603c8 100644 --- a/crates/jcode-base/src/auth/oauth.rs +++ b/crates/jcode-base/src/auth/oauth.rs @@ -433,13 +433,16 @@ pub async fn wait_for_callback_async_on_listener( /// Perform OAuth login for Claude pub async fn login_claude(no_browser: bool) -> Result { let (verifier, challenge) = generate_pkce(); + // SEC-01: the CSRF `state` is independent of the PKCE verifier. The verifier + // is a client secret and must never appear in the authorization URL. + let state = generate_state(); if let Ok(code) = std::env::var("JCODE_CLAUDE_AUTH_CODE") { let trimmed = code.trim(); if trimmed.is_empty() { anyhow::bail!("JCODE_CLAUDE_AUTH_CODE is set but empty"); } eprintln!("Exchanging code for tokens..."); - return exchange_claude_code(&verifier, trimmed, claude::REDIRECT_URI).await; + return exchange_claude_code(&verifier, &state, trimmed, claude::REDIRECT_URI).await; } if !std::io::stdin().is_terminal() { @@ -453,8 +456,8 @@ pub async fn login_claude(no_browser: bool) -> Result { let port = listener.local_addr()?.port(); let redirect_uri = format!("http://localhost:{}/callback", port); - let auth_url = claude_auth_url(&redirect_uri, &challenge, &verifier); - let manual_auth_url = claude_auth_url(claude::REDIRECT_URI, &challenge, &verifier); + let auth_url = claude_auth_url(&redirect_uri, &challenge, &state); + let manual_auth_url = claude_auth_url(claude::REDIRECT_URI, &challenge, &state); eprintln!("\nOpen this URL in your browser:\n"); eprintln!("{}\n", auth_url); @@ -485,13 +488,13 @@ pub async fn login_claude(no_browser: bool) -> Result { if browser_opened { match tokio::time::timeout( std::time::Duration::from_secs(120), - wait_for_callback_async_on_listener(listener, &verifier), + wait_for_callback_async_on_listener(listener, &state), ) .await { Ok(Ok(code)) => { eprintln!("Received callback. Exchanging code for tokens..."); - return exchange_claude_code(&verifier, &code, &redirect_uri).await; + return exchange_claude_code(&verifier, &state, &code, &redirect_uri).await; } Ok(Err(err)) => { eprintln!( @@ -515,11 +518,11 @@ pub async fn login_claude(no_browser: bool) -> Result { } eprintln!("Exchanging code for tokens..."); let selected_redirect_uri = claude_redirect_uri_for_input(trimmed, &redirect_uri); - return exchange_claude_code(&verifier, trimmed, &selected_redirect_uri).await; + return exchange_claude_code(&verifier, &state, trimmed, &selected_redirect_uri).await; } // Last-resort manual flow if localhost callback binding is unavailable. - let auth_url = claude_auth_url(claude::REDIRECT_URI, &challenge, &verifier); + let auth_url = claude_auth_url(claude::REDIRECT_URI, &challenge, &state); eprintln!("\nOpen this URL in your browser:\n"); eprintln!("{}\n", auth_url); @@ -546,7 +549,7 @@ pub async fn login_claude(no_browser: bool) -> Result { } eprintln!("Exchanging code for tokens..."); - exchange_claude_code(&verifier, trimmed, claude::REDIRECT_URI).await + exchange_claude_code(&verifier, &state, trimmed, claude::REDIRECT_URI).await } pub fn claude_auth_url(redirect_uri: &str, challenge: &str, state: &str) -> String { @@ -646,21 +649,24 @@ fn looks_like_cloudflare_challenge(text: &str) -> bool { async fn exchange_claude_code_at_url( token_url: &str, verifier: &str, + expected_state: &str, input: &str, redirect_uri: &str, ) -> Result { let (code, state_from_callback) = parse_claude_code_input(input)?; - // Anthropic's token endpoint expects `state`. - // We bind state to the PKCE verifier in the auth URL; if callback input - // includes a non-empty state, it must match to avoid CSRF or stale-code mixups. + // Anthropic's token endpoint expects `state`. The authorization URL now + // carries an independent CSRF `state` (SEC-01): the PKCE `code_verifier` is + // never placed in the URL. If the callback input includes a non-empty + // state, it must match the state we generated to avoid CSRF or stale-code + // mixups. let state = match state_from_callback.as_deref().filter(|s| !s.is_empty()) { - Some(callback_state) if callback_state != verifier => { + Some(callback_state) if callback_state != expected_state => { anyhow::bail!( "OAuth state mismatch. Start login again and use the latest callback/code." ) } Some(callback_state) => callback_state.to_string(), - None => verifier.to_string(), + None => expected_state.to_string(), }; #[derive(Serialize)] @@ -730,10 +736,18 @@ async fn exchange_claude_code_at_url( /// `input` can be a plain code, a URL/query containing `code=`, or `code#state`. pub async fn exchange_claude_code( verifier: &str, + expected_state: &str, input: &str, redirect_uri: &str, ) -> Result { - exchange_claude_code_at_url(claude::TOKEN_URL, verifier, input, redirect_uri).await + exchange_claude_code_at_url( + claude::TOKEN_URL, + verifier, + expected_state, + input, + redirect_uri, + ) + .await } pub fn openai_auth_url(redirect_uri: &str, challenge: &str, state: &str) -> String { diff --git a/crates/jcode-base/src/auth/oauth_tests/flow.rs b/crates/jcode-base/src/auth/oauth_tests/flow.rs index c1f354dd19..279b3c4f96 100644 --- a/crates/jcode-base/src/auth/oauth_tests/flow.rs +++ b/crates/jcode-base/src/auth/oauth_tests/flow.rs @@ -236,15 +236,9 @@ fn openai_refresh_request_targets_correct_url() -> Result<()> { #[test] fn claude_auth_url_contains_required_params() -> Result<()> { let (verifier, challenge) = generate_pkce(); - let auth_url = format!( - "{}?code=true&client_id={}&response_type=code&redirect_uri={}&scope={}&code_challenge={}&code_challenge_method=S256&state={}", - claude::AUTHORIZE_URL, - claude::CLIENT_ID, - urlencoding::encode(claude::REDIRECT_URI), - urlencoding::encode(claude::SCOPES), - challenge, - verifier, - ); + let state = generate_state(); + // Use the production builder so the test tracks real behaviour. + let auth_url = claude_auth_url(claude::REDIRECT_URI, &challenge, &state); let parsed = url::Url::parse(&auth_url).map_err(|e| anyhow!(e))?; let params: HashMap = parsed .query_pairs() @@ -260,7 +254,18 @@ fn claude_auth_url_contains_required_params() -> Result<()> { assert_eq!(require_param(¶ms, "scope")?, claude::SCOPES); assert_eq!(require_param(¶ms, "code_challenge")?, challenge); assert_eq!(require_param(¶ms, "code_challenge_method")?, "S256"); - assert_eq!(require_param(¶ms, "state")?, verifier); + assert_eq!(require_param(¶ms, "state")?, state); + // SEC-01 regression: the PKCE verifier must never be used as the CSRF state, + // so it must never appear verbatim in the authorization URL. + assert_ne!( + require_param(¶ms, "state")?, + verifier, + "PKCE code_verifier leaked into the authorization URL as state" + ); + assert!( + !auth_url.contains(&verifier), + "PKCE code_verifier must not appear anywhere in the authorization URL" + ); assert_eq!(parsed.host_str(), Some("claude.com")); assert_eq!(parsed.path(), "/cai/oauth/authorize"); Ok(()) @@ -513,6 +518,7 @@ async fn claude_exchange_uses_state_from_url_query_when_present() -> Result<()> let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); let _ = exchange_claude_code_at_url( &url, + "verifier", "query_state", "https://example.com/callback?code=test_code&state=query_state", "https://r", @@ -537,7 +543,8 @@ async fn claude_exchange_uses_claude_code_token_headers() -> Result<()> { let (port, handle) = mock_token_server(200, &success_body).await; let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); - let _ = exchange_claude_code_at_url(&url, "verifier", "plain_code", "https://r").await?; + let _ = + exchange_claude_code_at_url(&url, "verifier", "state", "plain_code", "https://r").await?; let (_method, _path, headers, _body) = handle.await.map_err(|e| anyhow!(e))?; assert_eq!( @@ -571,7 +578,7 @@ async fn claude_exchange_rejects_token_without_inference_scope() -> Result<()> { let (port, _handle) = mock_token_server(200, &success_body).await; let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); - let err = exchange_claude_code_at_url(&url, "verifier", "plain_code", "https://r") + let err = exchange_claude_code_at_url(&url, "verifier", "state", "plain_code", "https://r") .await .expect_err("token without user:inference should be rejected") .to_string(); @@ -593,7 +600,8 @@ async fn claude_exchange_preserves_returned_scopes() -> Result<()> { let (port, _handle) = mock_token_server(200, &success_body).await; let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); - let tokens = exchange_claude_code_at_url(&url, "verifier", "plain_code", "https://r").await?; + let tokens = + exchange_claude_code_at_url(&url, "verifier", "state", "plain_code", "https://r").await?; assert!(tokens.scopes.iter().any(|scope| scope == "user:inference")); Ok(()) @@ -605,7 +613,7 @@ async fn claude_exchange_cloudflare_403_is_actionable() -> Result<()> { let (port, _handle) = mock_token_server(403, challenge).await; let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); - let err = exchange_claude_code_at_url(&url, "verifier", "plain_code", "https://r") + let err = exchange_claude_code_at_url(&url, "verifier", "state", "plain_code", "https://r") .await .expect_err("Cloudflare challenge should fail with guidance") .to_string(); @@ -620,6 +628,7 @@ async fn claude_exchange_cloudflare_403_is_actionable() -> Result<()> { async fn claude_exchange_rejects_state_mismatch() -> Result<()> { let result = exchange_claude_code_at_url( "http://127.0.0.1:1/v1/oauth/token", + "verifier", "expected_state", "https://example.com/callback?code=test_code&state=wrong_state", "https://r", @@ -674,7 +683,7 @@ async fn openai_callback_input_rejects_state_mismatch() -> Result<()> { } #[tokio::test] -async fn claude_exchange_falls_back_to_verifier_when_input_has_no_state() -> Result<()> { +async fn claude_exchange_falls_back_to_expected_state_when_input_has_no_state() -> Result<()> { let success_body = serde_json::json!({ "access_token": "at", "refresh_token": "rt", @@ -684,17 +693,26 @@ async fn claude_exchange_falls_back_to_verifier_when_input_has_no_state() -> Res let (port, handle) = mock_token_server(200, &success_body).await; let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); - let _ = exchange_claude_code_at_url(&url, "verifier_only", "plain_code", "https://r").await?; + let _ = exchange_claude_code_at_url( + &url, + "verifier", + "generated_state", + "plain_code", + "https://r", + ) + .await?; let (_method, _path, _headers, body) = handle.await.map_err(|e| anyhow!(e))?; let body: serde_json::Value = serde_json::from_str(&body)?; - assert_eq!(require_json_str(&body, "state")?, "verifier_only"); + // With no state in the callback input, the exchange uses the state we + // generated for the auth URL (SEC-01) — never the PKCE verifier. + assert_eq!(require_json_str(&body, "state")?, "generated_state"); assert_eq!(require_json_str(&body, "code")?, "plain_code"); Ok(()) } #[tokio::test] -async fn claude_exchange_uses_verifier_when_input_state_is_empty() -> Result<()> { +async fn claude_exchange_uses_expected_state_when_input_state_is_empty() -> Result<()> { let success_body = serde_json::json!({ "access_token": "at", "refresh_token": "rt", @@ -704,11 +722,18 @@ async fn claude_exchange_uses_verifier_when_input_state_is_empty() -> Result<()> let (port, handle) = mock_token_server(200, &success_body).await; let url = format!("http://127.0.0.1:{}/v1/oauth/token", port); - let _ = exchange_claude_code_at_url(&url, "verifier_only", "plain_code#", "https://r").await?; + let _ = exchange_claude_code_at_url( + &url, + "verifier", + "generated_state", + "plain_code#", + "https://r", + ) + .await?; let (_method, _path, _headers, body) = handle.await.map_err(|e| anyhow!(e))?; let body: serde_json::Value = serde_json::from_str(&body)?; - assert_eq!(require_json_str(&body, "state")?, "verifier_only"); + assert_eq!(require_json_str(&body, "state")?, "generated_state"); Ok(()) } diff --git a/crates/jcode-base/src/config.rs b/crates/jcode-base/src/config.rs index 935248ea17..8b1aa63a19 100644 --- a/crates/jcode-base/src/config.rs +++ b/crates/jcode-base/src/config.rs @@ -824,6 +824,10 @@ mod tests; #[path = "config_color_tests.rs"] mod color_tests; +#[cfg(test)] +#[path = "config_atomic_save_tests.rs"] +mod atomic_save_tests; + /// Whether integration discovery settings carry no information beyond the shipped /// default, so `[sponsors]` can be left out of written config files. /// diff --git a/crates/jcode-base/src/config/config_file.rs b/crates/jcode-base/src/config/config_file.rs index 3d566ddc5b..e590470a02 100644 --- a/crates/jcode-base/src/config/config_file.rs +++ b/crates/jcode-base/src/config/config_file.rs @@ -1,6 +1,149 @@ use super::*; use crate::storage::jcode_dir; use std::path::PathBuf; +use std::sync::Mutex; + +/// Serializes all `Config::save()` writers in this process so concurrent +/// `load -> mutate -> save` helpers cannot clobber each other (RC-01). This is +/// intra-process; the atomic temp-file + `rename` in `save()` additionally makes +/// writes crash-safe and reduces (though cannot fully eliminate) cross-process +/// interleavings. +static CONFIG_WRITE_LOCK: Mutex<()> = Mutex::new(()); + +/// Last config that parsed successfully in this process (REL-02). +/// +/// When a later reload hits a malformed file, we return this instead of +/// `Config::default()` so a single TOML typo cannot silently wipe live user +/// settings (including security opt-outs). `None` until the first good load. +static LAST_GOOD_CONFIG: Mutex> = Mutex::new(None); + +/// Cross-process advisory lock over config writes (RC-01). +/// +/// Held for the whole read-modify-write in [`Config::mutate_if`] so two +/// separate jcode processes serialize their `load -> mutate -> save` cycles and +/// cannot lose one another's updates. On Unix this is a `flock(LOCK_EX)` on a +/// dedicated `config.toml.lock` file; on other platforms it is a no-op and only +/// the in-process mutex applies (documented limitation). Acquisition is +/// best-effort: a lock failure logs and proceeds rather than blocking config +/// writes, since the atomic rename still prevents a torn file. +struct ConfigFileLock { + /// Held open for the lock's lifetime on Unix and Windows; `None` when the + /// lock file could not be opened or on platforms with no advisory-lock + /// path (then only the in-process mutex applies). Unused otherwise. + #[cfg_attr(not(any(unix, windows)), allow(dead_code))] + file: Option, +} + +impl ConfigFileLock { + /// Open (creating if needed) and harden the `config.toml.lock` file. + fn open_lock_file() -> Option { + let lock_path = Config::path()?.with_extension("toml.lock"); + if let Some(parent) = lock_path.parent() { + let _ = std::fs::create_dir_all(parent); + } + let f = std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path) + .ok()?; + // Harden the lock file (it lives beside the secret-bearing config); + // ignore failures. + let _ = jcode_core::fs::set_permissions_owner_only(&lock_path); + Some(f) + } + + fn acquire() -> Self { + let file = Self::open_lock_file(); + + #[cfg(unix)] + if let Some(ref f) = file { + use std::os::unix::io::AsRawFd; + // Blocking exclusive advisory lock. flock retries EINTR itself. + let rc = unsafe { libc::flock(f.as_raw_fd(), libc::LOCK_EX) }; + if rc != 0 { + crate::logging::warn( + "config: could not acquire inter-process write lock; proceeding \ + with in-process lock only", + ); + } + } + + #[cfg(windows)] + if let Some(ref f) = file { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Storage::FileSystem::{LOCKFILE_EXCLUSIVE_LOCK, LockFileEx}; + use windows_sys::Win32::System::IO::OVERLAPPED; + // Blocking exclusive lock over the whole (0..u32::MAX,u32::MAX) + // range — the byte range is nominal since the file is empty; the + // lock is what serializes writers across processes. + let mut overlapped: OVERLAPPED = unsafe { std::mem::zeroed() }; + let ok = unsafe { + LockFileEx( + f.as_raw_handle() as _, + LOCKFILE_EXCLUSIVE_LOCK, + 0, + u32::MAX, + u32::MAX, + &mut overlapped, + ) + }; + if ok == 0 { + crate::logging::warn( + "config: could not acquire inter-process write lock; proceeding \ + with in-process lock only", + ); + } + } + + #[cfg(not(any(unix, windows)))] + { + // No advisory-lock API wired for this platform; the in-process mutex + // still serializes threads. Kept explicit rather than silent. + let _ = &file; + } + + ConfigFileLock { file } + } +} + +impl Drop for ConfigFileLock { + fn drop(&mut self) { + let Some(ref f) = self.file else { return }; + + #[cfg(unix)] + { + use std::os::unix::io::AsRawFd; + // Release the advisory lock; closing the fd would also drop it, but + // be explicit so the unlock is visible and prompt. + unsafe { + libc::flock(f.as_raw_fd(), libc::LOCK_UN); + } + } + + #[cfg(windows)] + { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Storage::FileSystem::UnlockFileEx; + use windows_sys::Win32::System::IO::OVERLAPPED; + let mut overlapped: OVERLAPPED = unsafe { std::mem::zeroed() }; + unsafe { + UnlockFileEx( + f.as_raw_handle() as _, + 0, + u32::MAX, + u32::MAX, + &mut overlapped, + ); + } + } + + #[cfg(not(any(unix, windows)))] + { + let _ = f; + } + } +} impl Config { /// Get the config file path @@ -25,24 +168,143 @@ impl Config { Ok(config) } - /// Load the on-disk config for a read-modify-write operation. + /// Load config from file only (no env overrides). /// - /// Unlike [`Self::load`], this never converts a parse error into defaults. - /// Saving those defaults would destroy the user's existing config. It also - /// deliberately skips environment overrides so transient process settings - /// are not baked into the file as a side effect of changing one preference. - fn load_for_update() -> anyhow::Result { - Ok(Self::load_from_file_strict()?.unwrap_or_default()) - } - - /// Load config from file only (no env overrides) + /// REL-02: on a parse/read error we must NOT silently fall through to + /// `Config::default()`, which would drop every user setting — including + /// security opt-outs like telemetry/discovery — the moment a single TOML + /// typo lands. Instead we (1) back up the corrupt file once so it is + /// recoverable and the user can repair it, and (2) return the last config + /// that loaded successfully in this process, so a bad edit does not reset + /// live settings. Interactive callers that need to surface the error use + /// [`Self::load_strict`] (see `config_edit_notice`). fn load_from_file() -> Option { match Self::load_from_file_strict() { - Ok(config) => config, + Ok(config) => { + if let Some(ref cfg) = config { + Self::remember_last_good(cfg); + } + config + } Err(e) => { - crate::logging::error(&format!("Failed to parse config file: {}", e)); - None + crate::logging::error(&format!( + "Failed to parse config file (keeping last-good settings; not resetting to defaults): {}", + e + )); + Self::back_up_corrupt_config(&e); + Self::last_good() + } + } + } + + /// Snapshot the most recently parsed-good config for REL-02 fallback, both + /// in-process and on disk. + /// + /// The in-process copy protects a running session; the on-disk copy + /// (`config.toml.last-good`) preserves the last known-good settings across + /// application restarts, so a fresh process that finds `config.toml` + /// corrupt recovers real settings instead of silently reverting to + /// `Config::default()`. The disk copy is a byte-for-byte snapshot of the + /// valid file (comments/formatting preserved) written 0o600 atomically. + fn remember_last_good(cfg: &Self) { + if let Ok(mut guard) = LAST_GOOD_CONFIG.lock() { + *guard = Some(cfg.clone()); + } + // Persist a raw snapshot of the just-validated file. Copy the on-disk + // bytes rather than re-serializing so comments and layout survive. + let Some(path) = Self::path() else { return }; + let Ok(raw) = std::fs::read(&path) else { + return; + }; + let snapshot = Self::last_good_path(); + // Skip the write when the snapshot already matches, to avoid churn. + if std::fs::read(&snapshot).ok().as_deref() == Some(raw.as_slice()) { + return; + } + if let Err(e) = Self::write_atomic_hardened(&snapshot, &raw) { + crate::logging::warn(&format!( + "Failed to persist last-good config snapshot to {}: {}", + snapshot.display(), + e + )); + } + } + + /// Path to the on-disk last-good config snapshot (REL-02). + fn last_good_path() -> std::path::PathBuf { + // Fall back to a relative name only if the primary path is unavailable; + // callers guard on that separately. + Self::path() + .map(|p| p.with_extension("toml.last-good")) + .unwrap_or_else(|| std::path::PathBuf::from("config.toml.last-good")) + } + + /// Test-only: clear the in-process last-good snapshot to simulate a fresh + /// process, so tests can exercise the on-disk restore path (REL-02). + #[cfg(test)] + pub(crate) fn clear_in_process_last_good_for_tests() { + if let Ok(mut guard) = LAST_GOOD_CONFIG.lock() { + *guard = None; + } + } + + /// The last config that parsed successfully — the in-process snapshot if + /// present, otherwise the on-disk `config.toml.last-good` from a prior run. + /// + /// The on-disk fallback is what makes REL-02 survive restarts: on a fresh + /// process with a malformed `config.toml`, this returns the persisted + /// known-good settings instead of `None` (which would become defaults). + fn last_good() -> Option { + if let Some(cfg) = LAST_GOOD_CONFIG.lock().ok().and_then(|g| g.clone()) { + return Some(cfg); + } + // Restore from the on-disk snapshot written by a previous good load. + let snapshot = Self::last_good_path(); + let content = std::fs::read_to_string(&snapshot).ok()?; + match toml::from_str::(&content) { + Ok(mut cfg) => { + cfg.display.apply_legacy_compat(); + cfg.repair_frozen_sponsors_optout(&content); + crate::logging::warn(&format!( + "config.toml was unreadable; recovered last-good settings from {}.", + snapshot.display() + )); + Some(cfg) } + // A corrupt snapshot is useless; let the caller fall through to defaults. + Err(_) => None, + } + } + + /// Copy a corrupt config file aside so the user can inspect/repair it and so + /// the bad content is never silently overwritten by the next `save()`. + /// + /// Idempotent per corrupt version: the backup is only (re)written when its + /// contents differ from the current corrupt file, so a repeated reload loop + /// does not churn the disk. + fn back_up_corrupt_config(error: &anyhow::Error) { + let Some(path) = Self::path() else { return }; + let Ok(corrupt) = std::fs::read(&path) else { + return; + }; + let backup = path.with_extension("toml.corrupt"); + if std::fs::read(&backup).ok().as_deref() == Some(corrupt.as_slice()) { + return; // already backed up this exact corrupt content + } + // Use the same secure temp-file-then-rename path as save() so the backup + // is created 0o600 BEFORE the (possibly key-bearing) corrupt bytes are + // written — never a window at the default umask (SEC-04 hardening gap). + match Self::write_atomic_hardened(&backup, &corrupt) { + Ok(()) => crate::logging::warn(&format!( + "Backed up unparseable config to {} so it can be repaired ({}).", + backup.display(), + error + )), + Err(e) => crate::logging::error(&format!( + "Failed to back up unparseable config to {}: {}", + backup.display(), + e + )), } } @@ -104,21 +366,117 @@ impl Config { ); } - /// Save config to file + /// Save config to file. + /// + /// The write is **atomic** and **hardened**: content goes to a temp file in + /// the same directory, is fsynced, then `rename()`d over the target so a + /// crash mid-write can never leave a truncated `config.toml`; the file is + /// `0o600` inside a `0o700` directory (SEC-04) because it may hold provider + /// `api_key`s. A process-wide lock serializes the physical write. + /// + /// NOTE (RC-01): `save()` alone does not make a `load -> mutate -> save` + /// sequence race-free — two callers can each load the old state and then + /// serialize only at write time, losing one update. Mutation paths must go + /// through [`Self::mutate`], which holds the lock across the whole cycle. pub fn save(&self) -> anyhow::Result<()> { - let path = Self::path().ok_or_else(|| anyhow::anyhow!("No config path"))?; + let _guard = CONFIG_WRITE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + // Serialize direct physical writes with mutate_if() writers in other + // processes too. A caller that needs an atomic read-modify-write must + // still use mutate()/mutate_if() so the read also occurs under the lock. + let _file_lock = ConfigFileLock::acquire(); + self.save_locked() + } - // Ensure parent directory exists + /// Physical atomic+hardened write. Caller must hold [`CONFIG_WRITE_LOCK`]. + fn save_locked(&self) -> anyhow::Result<()> { + let path = Self::path().ok_or_else(|| anyhow::anyhow!("No config path"))?; if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; } - let content = toml::to_string_pretty(self)?; - std::fs::write(&path, content)?; + Self::write_atomic_hardened(&path, content.as_bytes())?; Self::invalidate_cache(); Ok(()) } + /// Atomically read-modify-write the config (RC-01). + /// + /// Holds the process-wide write lock across the entire `load -> apply -> + /// save` cycle so concurrent mutations of disjoint fields cannot clobber + /// each other. `f` receives the freshly loaded config and mutates it in + /// place; the result is persisted atomically before the lock is released. + pub fn mutate(f: impl FnOnce(&mut Self)) -> anyhow::Result<()> { + Self::mutate_if(|cfg| { + f(cfg); + true + }) + } + + /// Like [`Self::mutate`], but only persists when the closure returns `true`. + /// + /// Lets callers keep an "only write when something actually changed" + /// optimization while still performing the whole read-modify-decide-write + /// cycle under the write lock (RC-01). Returns `Ok(())` whether or not a + /// write occurred. + pub fn mutate_if(f: impl FnOnce(&mut Self) -> bool) -> anyhow::Result<()> { + // Intra-process: serialize threads cheaply. + let _guard = CONFIG_WRITE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + // Inter-process (RC-01): hold an advisory file lock across the whole + // read-modify-write so two separate jcode processes cannot each load, + // edit disjoint fields, and clobber one another. Best-effort: if the + // lock cannot be taken we proceed (never worse than before, and the + // atomic rename still prevents a torn file). + let _flock = ConfigFileLock::acquire(); + // Load fresh from disk INSIDE both locks so we never mutate a stale copy. + let mut cfg = Self::load(); + if f(&mut cfg) { + cfg.save_locked()?; + } + Ok(()) + } + + /// Atomically write `bytes` to `path` with owner-only permissions. + /// + /// Temp-file-plus-rename gives crash safety (RC-01); the `0o600`/`0o700` + /// hardening gives secret-file protection for in-file API keys (SEC-04). + fn write_atomic_hardened(path: &std::path::Path, bytes: &[u8]) -> anyhow::Result<()> { + use std::io::Write; + let parent = path + .parent() + .ok_or_else(|| anyhow::anyhow!("config path has no parent directory"))?; + + // Temp file in the SAME directory so `rename` stays on one filesystem + // (cross-device rename is not atomic and would fall back to copy). + let mut tmp = tempfile::Builder::new() + .prefix(".config.toml.") + .suffix(".tmp") + .tempfile_in(parent)?; + + // Harden the temp file BEFORE it holds secrets, so there is never a + // window where the key-bearing bytes sit at default (readable) perms. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + tmp.as_file() + .set_permissions(std::fs::Permissions::from_mode(0o600))?; + } + + tmp.write_all(bytes)?; + tmp.flush()?; + // Durability: flush the file's contents to disk before the rename so a + // crash cannot expose an empty/torn config (REL-02 defense in depth). + tmp.as_file().sync_all()?; + + // Atomic replace. + tmp.persist(path) + .map_err(|e| anyhow::anyhow!("failed to persist config file: {}", e.error))?; + + // Best-effort hardening of the final file + parent dir (covers Windows + // ACLs and tightens a pre-existing permissive directory). + crate::storage::harden_secret_file_permissions(path); + Ok(()) + } + /// Mark the process-cached config as stale and notify dependent caches. pub fn invalidate_cache() { super::invalidate_config_cache(); @@ -127,9 +485,7 @@ impl Config { /// Update the copilot premium mode in the config file. /// Reloads, patches, and saves so it doesn't clobber other fields. pub fn set_copilot_premium(mode: Option<&str>) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.provider.copilot_premium = mode.map(|s| s.to_string()); - cfg.save()?; + Self::mutate(|cfg| cfg.provider.copilot_premium = mode.map(|s| s.to_string()))?; crate::logging::info(&format!( "Saved copilot_premium to config: {}", mode.unwrap_or("(none)") @@ -140,10 +496,10 @@ impl Config { /// Update just the default model and provider in the config file. /// This reloads, patches, and saves so it doesn't clobber other fields. pub fn set_default_model(model: Option<&str>, provider: Option<&str>) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.provider.default_model = model.map(|s| s.to_string()); - cfg.provider.default_provider = provider.map(|s| s.to_string()); - cfg.save()?; + Self::mutate(|cfg| { + cfg.provider.default_model = model.map(|s| s.to_string()); + cfg.provider.default_provider = provider.map(|s| s.to_string()); + })?; crate::logging::info(&format!( "Saved default model: {}, provider: {}", model.unwrap_or("(none)"), @@ -154,21 +510,17 @@ impl Config { /// Update just the default provider in the config file. pub fn set_default_provider(provider: Option<&str>) -> anyhow::Result<()> { - let cfg = Self::load_for_update()?; - Self::set_default_model(cfg.provider.default_model.as_deref(), provider) + Self::mutate(|cfg| cfg.provider.default_provider = provider.map(|s| s.to_string())) } /// Update just the default model in the config file. pub fn set_default_model_only(model: Option<&str>) -> anyhow::Result<()> { - let cfg = Self::load_for_update()?; - Self::set_default_model(model, cfg.provider.default_provider.as_deref()) + Self::mutate(|cfg| cfg.provider.default_model = model.map(|s| s.to_string())) } /// Update the persisted OpenAI reasoning effort preference. pub fn set_openai_reasoning_effort(value: Option<&str>) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.provider.openai_reasoning_effort = value.map(|s| s.to_string()); - cfg.save()?; + Self::mutate(|cfg| cfg.provider.openai_reasoning_effort = value.map(|s| s.to_string()))?; crate::logging::info(&format!( "Saved openai_reasoning_effort to config: {}", value.unwrap_or("(none)") @@ -178,9 +530,7 @@ impl Config { /// Update the persisted Anthropic reasoning effort preference. pub fn set_anthropic_reasoning_effort(value: Option<&str>) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.provider.anthropic_reasoning_effort = value.map(|s| s.to_string()); - cfg.save()?; + Self::mutate(|cfg| cfg.provider.anthropic_reasoning_effort = value.map(|s| s.to_string()))?; crate::logging::info(&format!( "Saved anthropic_reasoning_effort to config: {}", value.unwrap_or("(none)") @@ -190,9 +540,7 @@ impl Config { /// Update the persisted OpenAI transport preference. pub fn set_openai_transport(value: Option<&str>) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.provider.openai_transport = value.map(|s| s.to_string()); - cfg.save()?; + Self::mutate(|cfg| cfg.provider.openai_transport = value.map(|s| s.to_string()))?; crate::logging::info(&format!( "Saved openai_transport to config: {}", value.unwrap_or("(none)") @@ -202,9 +550,7 @@ impl Config { /// Update the persisted OpenAI service tier preference. pub fn set_openai_service_tier(value: Option<&str>) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.provider.openai_service_tier = value.map(|s| s.to_string()); - cfg.save()?; + Self::mutate(|cfg| cfg.provider.openai_service_tier = value.map(|s| s.to_string()))?; crate::logging::info(&format!( "Saved openai_service_tier to config: {}", value.unwrap_or("(none)") @@ -214,18 +560,14 @@ impl Config { /// Update the persisted default alignment preference. pub fn set_display_centered(centered: bool) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.display.centered = centered; - cfg.save()?; + Self::mutate(|cfg| cfg.display.centered = centered)?; crate::logging::info(&format!("Saved display.centered to config: {}", centered)); Ok(()) } /// Update the persisted reasoning display mode preference. pub fn set_reasoning_display(mode: ReasoningDisplayMode) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.display.set_reasoning_display(mode); - cfg.save()?; + Self::mutate(|cfg| cfg.display.set_reasoning_display(mode))?; crate::logging::info(&format!( "Saved display.reasoning_display to config: {}", mode.label() @@ -235,9 +577,7 @@ impl Config { /// Update the persisted compact-notifications preference. pub fn set_compact_notifications(compact: bool) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.display.compact_notifications = compact; - cfg.save()?; + Self::mutate(|cfg| cfg.display.compact_notifications = compact)?; crate::logging::info(&format!( "Saved display.compact_notifications to config: {}", compact @@ -247,18 +587,14 @@ impl Config { /// Update the persisted pinned-todos preference. pub fn set_pin_todos(pin: bool) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.display.pin_todos = pin; - cfg.save()?; + Self::mutate(|cfg| cfg.display.pin_todos = pin)?; crate::logging::info(&format!("Saved display.pin_todos to config: {}", pin)); Ok(()) } /// Update the persisted show-agentgrep-output preference. pub fn set_show_agentgrep_output(show: bool) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.display.show_agentgrep_output = show; - cfg.save()?; + Self::mutate(|cfg| cfg.display.show_agentgrep_output = show)?; crate::logging::info(&format!( "Saved display.show_agentgrep_output to config: {}", show @@ -268,9 +604,7 @@ impl Config { /// Update the persisted tool-call-details preference. pub fn set_tool_call_details(show: bool) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.display.tool_call_details = show; - cfg.save()?; + Self::mutate(|cfg| cfg.display.tool_call_details = show)?; crate::logging::info(&format!( "Saved display.tool_call_details to config: {}", show @@ -287,14 +621,14 @@ impl Config { entries: Vec, enabled: bool, ) -> anyhow::Result<()> { - let mut cfg = Self::load_for_update()?; - cfg.launch_hotkeys.entries = entries; - cfg.launch_hotkeys.enabled = Some(enabled); - cfg.launch_hotkeys.imported = true; - cfg.save()?; + let entry_count = entries.len(); + Self::mutate(|cfg| { + cfg.launch_hotkeys.entries = entries; + cfg.launch_hotkeys.enabled = Some(enabled); + cfg.launch_hotkeys.imported = true; + })?; crate::logging::info(&format!( - "Saved {} launch hotkey(s) to config (enabled={enabled})", - cfg.launch_hotkeys.entries.len() + "Saved {entry_count} launch hotkey(s) to config (enabled={enabled})" )); Ok(()) } @@ -665,18 +999,20 @@ impl Config { anyhow::bail!("External auth source id cannot be empty"); } - let mut cfg = Self::load_for_update()?; - if !cfg - .auth - .trusted_external_sources - .iter() - .any(|value| value.trim().eq_ignore_ascii_case(&source_id)) - { + Self::mutate_if(|cfg| { + if cfg + .auth + .trusted_external_sources + .iter() + .any(|value| value.trim().eq_ignore_ascii_case(&source_id)) + { + return false; + } cfg.auth.trusted_external_sources.push(source_id.clone()); cfg.auth.trusted_external_sources.sort(); cfg.auth.trusted_external_sources.dedup(); - cfg.save()?; - } + true + })?; crate::logging::info(&format!( "Saved trusted external auth source to config: {}", @@ -690,18 +1026,20 @@ impl Config { path: &std::path::Path, ) -> anyhow::Result<()> { let entry = Self::trusted_external_auth_path_entry(source_id, path)?; - let mut cfg = Self::load_for_update()?; - if !cfg - .auth - .trusted_external_source_paths - .iter() - .any(|value| value.trim().eq_ignore_ascii_case(&entry)) - { + Self::mutate_if(|cfg| { + if cfg + .auth + .trusted_external_source_paths + .iter() + .any(|value| value.trim().eq_ignore_ascii_case(&entry)) + { + return false; + } cfg.auth.trusted_external_source_paths.push(entry.clone()); cfg.auth.trusted_external_source_paths.sort(); cfg.auth.trusted_external_source_paths.dedup(); - cfg.save()?; - } + true + })?; crate::logging::info(&format!( "Saved trusted external auth source path: {}", entry @@ -714,19 +1052,20 @@ impl Config { path: &std::path::Path, ) -> anyhow::Result<()> { let entry = Self::trusted_external_auth_path_entry(source_id, path)?; - let mut cfg = Self::load_for_update()?; - let before = cfg.auth.trusted_external_source_paths.len(); - cfg.auth - .trusted_external_source_paths - .retain(|value| !value.trim().eq_ignore_ascii_case(&entry)); - if cfg.auth.trusted_external_source_paths.len() != before { - cfg.save()?; - crate::logging::info(&format!( - "Removed trusted external auth source path: {}", - entry - )); - } - Ok(()) + Self::mutate_if(|cfg| { + let before = cfg.auth.trusted_external_source_paths.len(); + cfg.auth + .trusted_external_source_paths + .retain(|value| !value.trim().eq_ignore_ascii_case(&entry)); + let changed = cfg.auth.trusted_external_source_paths.len() != before; + if changed { + crate::logging::info(&format!( + "Removed trusted external auth source path: {}", + entry + )); + } + changed + }) } /// Remove a source-level (non-path) trust decision, e.g. for credentials @@ -736,102 +1075,19 @@ impl Config { if source_id.is_empty() { return Ok(()); } - let mut cfg = Self::load_for_update()?; - let before = cfg.auth.trusted_external_sources.len(); - cfg.auth - .trusted_external_sources - .retain(|value| !value.trim().eq_ignore_ascii_case(&source_id)); - if cfg.auth.trusted_external_sources.len() != before { - cfg.save()?; - crate::logging::info(&format!( - "Removed trusted external auth source: {}", - source_id - )); - } - Ok(()) - } -} - -#[cfg(test)] -mod issue_1056_tests { - use super::Config; - - struct EnvGuard { - key: &'static str, - previous: Option, - } - - impl EnvGuard { - fn set(key: &'static str, value: impl AsRef) -> Self { - let previous = std::env::var_os(key); - crate::env::set_var(key, value); - Self { key, previous } - } - } - - impl Drop for EnvGuard { - fn drop(&mut self) { - match self.previous.take() { - Some(value) => crate::env::set_var(self.key, value), - None => crate::env::remove_var(self.key), + Self::mutate_if(|cfg| { + let before = cfg.auth.trusted_external_sources.len(); + cfg.auth + .trusted_external_sources + .retain(|value| !value.trim().eq_ignore_ascii_case(&source_id)); + let changed = cfg.auth.trusted_external_sources.len() != before; + if changed { + crate::logging::info(&format!( + "Removed trusted external auth source: {}", + source_id + )); } - Config::invalidate_cache(); - } - } - - #[test] - fn effort_update_preserves_profile_with_capitalized_bearer_auth() { - let _lock = crate::storage::lock_test_env(); - let home = tempfile::tempdir().unwrap(); - let _home = EnvGuard::set("JCODE_HOME", home.path()); - let path = home.path().join("config.toml"); - std::fs::write( - &path, - r#" -[provider] -openai_reasoning_effort = "low" - -[providers.mistral] -type = "openai-compatible" -base_url = "https://api.mistral.ai/v1" -auth = "Bearer" -api_key_env = "MISTRAL_API_KEY" -disable_reasoning_heuristics = true - -[[providers.mistral.models]] -id = "mistral-medium-latest" -reasoning = true -reasoning_effort = "max" -"#, - ) - .unwrap(); - - Config::set_openai_reasoning_effort(Some("high")).unwrap(); - - let saved = std::fs::read_to_string(path).unwrap(); - assert!(saved.contains("[providers.mistral]")); - assert!(saved.contains("mistral-medium-latest")); - let parsed = Config::load_strict().unwrap(); - assert_eq!( - parsed.provider.openai_reasoning_effort.as_deref(), - Some("high") - ); - assert_eq!(parsed.providers["mistral"].models.len(), 1); - } - - #[test] - fn effort_update_refuses_to_overwrite_malformed_config() { - let _lock = crate::storage::lock_test_env(); - let home = tempfile::tempdir().unwrap(); - let _home = EnvGuard::set("JCODE_HOME", home.path()); - let path = home.path().join("config.toml"); - let original = "[providers.broken]\nauth = \"invalid-auth-mode\"\n"; - std::fs::write(&path, original).unwrap(); - - let error = Config::set_openai_reasoning_effort(Some("high")) - .expect_err("a malformed config must block mutation"); - - assert!(error.to_string().contains("Failed to parse config file")); - assert_eq!(std::fs::read_to_string(path).unwrap(), original); + changed + }) } } diff --git a/crates/jcode-base/src/config_atomic_save_tests.rs b/crates/jcode-base/src/config_atomic_save_tests.rs new file mode 100644 index 0000000000..7b5848cd08 --- /dev/null +++ b/crates/jcode-base/src/config_atomic_save_tests.rs @@ -0,0 +1,237 @@ +//! RC-01 + SEC-04 tests: config saves must be atomic and owner-only. +//! +//! RC-01 (lost-update race): every `set_*` helper does `load -> mutate -> save`, +//! and `save()` must serialize writers + write atomically so two concurrent +//! mutations of disjoint fields cannot clobber each other or leave a torn file. +//! +//! SEC-04 (plaintext key exposure): `config.toml` can hold provider `api_key`s, +//! so the written file must be `0o600` (owner-only) rather than default perms. + +use super::Config; + +/// A freshly saved config file must be owner-only (SEC-04). +#[cfg(unix)] +#[test] +fn saved_config_file_is_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + Config::invalidate_cache(); + + // config.toml can carry provider secrets (named-provider `api_key`, bing + // key, ...), so the written file must be owner-only regardless of contents. + Config::default().save().expect("save config"); + + let path = Config::path().expect("config path"); + let mode = std::fs::metadata(&path) + .expect("stat config") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o600, + "config.toml with an api_key must be owner-only" + ); + + // The parent directory should also be owner-only. + let dir_mode = std::fs::metadata(path.parent().unwrap()) + .expect("stat dir") + .permissions() + .mode() + & 0o777; + assert_eq!(dir_mode, 0o700, "config dir must be owner-only"); + + match prev_home { + Some(prev) => crate::env::set_var("JCODE_HOME", prev), + None => crate::env::remove_var("JCODE_HOME"), + } + Config::invalidate_cache(); +} + +/// Concurrent `set_*` helpers touching disjoint fields must not lose updates +/// (RC-01). Before the fix, `load -> mutate -> save` racing on two fields would +/// drop one write; the write lock + atomic rename must preserve both. +#[test] +fn concurrent_disjoint_writes_do_not_lose_updates() { + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + Config::invalidate_cache(); + + // Seed a file so both writers start from the same on-disk state. + Config::default().save().expect("seed save"); + + // Hammer two disjoint fields from many threads. Each call is a full + // load->mutate->save cycle, exactly the racy pattern RC-01 describes. + let threads: Vec<_> = (0..8) + .map(|i| { + std::thread::spawn(move || { + if i % 2 == 0 { + Config::set_default_model_only(Some("model-x")).expect("set model"); + } else { + Config::set_display_centered(true).expect("set centered"); + } + }) + }) + .collect(); + for t in threads { + t.join().expect("thread join"); + } + + // The file must remain parseable (no torn write) and reflect BOTH last + // writes, not just whichever raced last. + Config::invalidate_cache(); + let loaded = Config::load_strict().expect("config must stay parseable after concurrent saves"); + assert_eq!( + loaded.provider.default_model.as_deref(), + Some("model-x"), + "the model write must survive the race" + ); + assert!( + loaded.display.centered, + "the centered write must survive the race" + ); + + match prev_home { + Some(prev) => crate::env::set_var("JCODE_HOME", prev), + None => crate::env::remove_var("JCODE_HOME"), + } + Config::invalidate_cache(); +} + +/// REL-02: a malformed config file must NOT silently reset live settings to +/// defaults; the last good config is preserved and the bad file is backed up. +#[test] +fn malformed_config_preserves_last_good_and_backs_up() { + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + Config::invalidate_cache(); + + // Establish a known-good on-disk + last-good state with a non-default value. + Config::set_display_centered(true).expect("seed good config"); + let good = Config::load(); + assert!( + good.display.centered, + "precondition: good config has centered=true" + ); + + // Corrupt the file with invalid TOML. + let path = Config::path().expect("config path"); + std::fs::write( + &path, + "this = is = not valid toml +[[[", + ) + .expect("write corrupt"); + + // Loading now must fall back to the last-good config, NOT Config::default(). + let recovered = Config::load(); + assert!( + recovered.display.centered, + "malformed config must preserve last-good settings, not reset to defaults" + ); + + // The corrupt file must be backed up for repair. + let backup = path.with_extension("toml.corrupt"); + assert!( + backup.exists(), + "corrupt config should be backed up to {backup:?}" + ); + let backed = std::fs::read_to_string(&backup).expect("read backup"); + assert!( + backed.contains("not valid toml"), + "backup must hold the corrupt bytes" + ); + + // The corrupt backup may contain API keys, so it must be owner-only, and it + // must never sit at default perms even briefly (created 0o600, not hardened + // after the write). + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&backup) + .expect("stat backup") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "corrupt-config backup must be owner-only"); + } + + // REL-02 across a restart: a fresh process has no in-process snapshot. After + // clearing it, load() must recover from the on-disk config.toml.last-good + // written during the earlier good load — NOT fall back to defaults. + Config::clear_in_process_last_good_for_tests(); + let last_good_snapshot = path.with_extension("toml.last-good"); + assert!( + last_good_snapshot.exists(), + "a good load must persist {last_good_snapshot:?} for restart recovery" + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&last_good_snapshot) + .expect("stat snapshot") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "last-good snapshot must be owner-only"); + } + Config::invalidate_cache(); + let after_restart = Config::load(); + assert!( + after_restart.display.centered, + "on a fresh process with a corrupt config, last-good must be recovered from disk" + ); + + match prev_home { + Some(prev) => crate::env::set_var("JCODE_HOME", prev), + None => crate::env::remove_var("JCODE_HOME"), + } + Config::invalidate_cache(); +} + +/// RC-01 inter-process: `mutate` acquires and releases the advisory file lock +/// without deadlocking, and the lock file is created owner-only. (True +/// cross-process serialization is exercised by the flock itself; here we prove +/// the lock is taken, reentered across sequential calls, and hardened.) +#[test] +fn config_write_lock_file_is_created_and_reusable() { + let _guard = crate::storage::lock_test_env(); + let prev_home = std::env::var_os("JCODE_HOME"); + let dir = tempfile::TempDir::new().expect("tempdir"); + crate::env::set_var("JCODE_HOME", dir.path()); + Config::invalidate_cache(); + + // Two sequential mutations must both succeed (lock released between them). + Config::set_display_centered(true).expect("first mutate"); + Config::set_display_centered(false).expect("second mutate after lock release"); + + let path = Config::path().expect("config path"); + let lock = path.with_extension("toml.lock"); + assert!( + lock.exists(), + "inter-process lock file should exist at {lock:?}" + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&lock) + .expect("stat lock") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "config lock file must be owner-only"); + } + + match prev_home { + Some(prev) => crate::env::set_var("JCODE_HOME", prev), + None => crate::env::remove_var("JCODE_HOME"), + } + Config::invalidate_cache(); +} diff --git a/crates/jcode-tui-style/src/palette.rs b/crates/jcode-tui-style/src/palette.rs index 38e3e88175..08af545d9f 100644 --- a/crates/jcode-tui-style/src/palette.rs +++ b/crates/jcode-tui-style/src/palette.rs @@ -378,6 +378,46 @@ pub fn adapt_buffer_for_palette(buf: &mut ratatui::buffer::Buffer) { } } +/// Whether the user has requested no color output (VC-01 / accessibility). +/// +/// Honors the cross-tool `NO_COLOR` convention (https://no-color.org) and +/// jcode's own `JCODE_NO_COLOR`, matching what the CLI already respects. Read +/// once and cached: env vars do not change mid-process, and this runs per +/// frame on the render hot path. +pub fn no_color_requested() -> bool { + use std::sync::OnceLock; + static NO_COLOR: OnceLock = OnceLock::new(); + *NO_COLOR.get_or_init(|| { + std::env::var_os("NO_COLOR").is_some() || std::env::var_os("JCODE_NO_COLOR").is_some() + }) +} + +/// Strip all foreground/background/underline colors from a rendered buffer when +/// `NO_COLOR`/`JCODE_NO_COLOR` is set (VC-01). +/// +/// The TUI's design tokens are structural (glyphs, spinners, layout) as well as +/// chromatic, so dropping color to the terminal default keeps the UI legible +/// and monochrome for low-vision users and screen-scraping tools, mirroring the +/// CLI's existing `NO_COLOR` handling. Text modifiers (bold/underline/reverse) +/// are preserved so emphasis survives without color. This is the render-loop +/// companion to [`adapt_buffer_for_palette`] and, unlike it, always applies. +pub fn strip_colors_for_no_color(buf: &mut ratatui::buffer::Buffer) { + strip_colors_if(buf, no_color_requested()); +} + +/// Core of [`strip_colors_for_no_color`] with the decision injected, so tests +/// can exercise both branches without mutating process-global env state. +fn strip_colors_if(buf: &mut ratatui::buffer::Buffer, strip: bool) { + if !strip { + return; + } + for cell in buf.content.iter_mut() { + cell.fg = Color::Reset; + cell.bg = Color::Reset; + cell.underline_color = Color::Reset; + } +} + /// The RGB a role's default renders as in the *current* theme. /// /// Literals arriving at substitution have already been through the light-theme @@ -646,6 +686,27 @@ mod buffer_tests { buf } + #[test] + fn no_color_strips_all_colors_to_reset() { + // VC-01: with NO_COLOR active, every fg/bg/underline drops to Reset. + let mut buf = buffer_with(&[Color::Rgb(255, 0, 0), Color::Indexed(42)]); + buf.content[0].bg = Color::Rgb(0, 0, 255); + buf.content[0].underline_color = Color::Green; + strip_colors_if(&mut buf, true); + for cell in buf.content.iter() { + assert_eq!(cell.fg, Color::Reset); + assert_eq!(cell.bg, Color::Reset); + assert_eq!(cell.underline_color, Color::Reset); + } + } + + #[test] + fn no_color_disabled_leaves_colors_untouched() { + let mut buf = buffer_with(&[Color::Rgb(255, 0, 0)]); + strip_colors_if(&mut buf, false); + assert_eq!(buf.content[0].fg, Color::Rgb(255, 0, 0)); + } + // The default palette must render byte-identically to the historical // hard-coded look. This is the regression that would silently recolor // every existing user's terminal. diff --git a/crates/jcode-tui/src/tui/app/auth.rs b/crates/jcode-tui/src/tui/app/auth.rs index 0f27681196..14344f0558 100644 --- a/crates/jcode-tui/src/tui/app/auth.rs +++ b/crates/jcode-tui/src/tui/app/auth.rs @@ -869,11 +869,14 @@ impl App { let hash = hasher.finalize(); let challenge = URL_SAFE_NO_PAD.encode(hash); + // SEC-01: independent CSRF state; the PKCE verifier must never appear in + // the authorization URL or be used as the OAuth `state`. + let state = crate::auth::oauth::generate_state_public(); + // Try a loopback callback first so the user never has to copy/paste the - // authorization code (mirrors the OpenAI/Gemini flows). Claude uses the - // PKCE verifier as the OAuth `state`, so we wait for that on the - // listener. If binding fails we fall back to manual paste with the - // hosted redirect page. + // authorization code (mirrors the OpenAI/Gemini flows). We wait for our + // generated `state` on the listener. If binding fails we fall back to + // manual paste with the hosted redirect page. let callback_listener = crate::auth::oauth::bind_callback_listener(0).ok(); let callback_port = callback_listener .as_ref() @@ -885,13 +888,13 @@ impl App { Some(port) if callback_available => { let redirect_uri = format!("http://localhost:{}/callback", port); let auth_url = - crate::auth::oauth::claude_auth_url(&redirect_uri, &challenge, &verifier); + crate::auth::oauth::claude_auth_url(&redirect_uri, &challenge, &state); (auth_url, redirect_uri) } _ => { let redirect_uri = crate::auth::oauth::claude::REDIRECT_URI.to_string(); let auth_url = - crate::auth::oauth::claude_auth_url(&redirect_uri, &challenge, &verifier); + crate::auth::oauth::claude_auth_url(&redirect_uri, &challenge, &state); (auth_url, redirect_uri) } }; @@ -915,11 +918,13 @@ impl App { // identically. if let (Some(listener), true) = (callback_listener, callback_available) { let verifier_clone = verifier.clone(); + let state_clone = state.clone(); let label_clone = label.to_string(); let redirect_clone = redirect_uri.clone(); tokio::spawn(async move { match Self::claude_login_callback( verifier_clone, + state_clone, label_clone, redirect_clone, listener, @@ -979,6 +984,7 @@ impl App { } self.begin_pending_login(PendingLogin::ClaudeAccount { verifier, + expected_state: state, label: label.to_string(), redirect_uri: if callback_available { Some(redirect_uri) @@ -990,20 +996,22 @@ impl App { async fn claude_login_callback( verifier: String, + expected_state: String, label: String, redirect_uri: String, listener: tokio::net::TcpListener, ) -> Result { - // Claude uses the PKCE verifier as the OAuth `state` value. + // SEC-01: wait for our independent CSRF state, not the PKCE verifier. let code = tokio::time::timeout( std::time::Duration::from_secs(300), - crate::auth::oauth::wait_for_callback_async_on_listener(listener, &verifier), + crate::auth::oauth::wait_for_callback_async_on_listener(listener, &expected_state), ) .await .map_err(|_| "Login timed out after 5 minutes. Please try again.".to_string())? .map_err(|e| format!("Callback failed: {}", e))?; - Self::claude_token_exchange(verifier, code, &label, Some(redirect_uri)).await + Self::claude_token_exchange(verifier, expected_state, code, &label, Some(redirect_uri)) + .await } pub(super) fn switch_account(&mut self, label: &str) { @@ -2062,6 +2070,7 @@ impl App { match pending { PendingLogin::ClaudeAccount { verifier, + expected_state, label, redirect_uri, } => { @@ -2071,6 +2080,7 @@ impl App { tokio::spawn(async move { match Self::claude_token_exchange( verifier, + expected_state, input_owned, &label_clone, redirect_uri, @@ -3327,6 +3337,7 @@ impl App { async fn claude_token_exchange( verifier: String, + expected_state: String, input: String, label: &str, redirect_uri: Option, @@ -3335,10 +3346,14 @@ impl App { redirect_uri.unwrap_or_else(|| crate::auth::oauth::claude::REDIRECT_URI.to_string()); let redirect_uri = crate::auth::oauth::claude_redirect_uri_for_input(input.trim(), &fallback_redirect_uri); - let oauth_tokens = - crate::auth::oauth::exchange_claude_code(&verifier, input.trim(), &redirect_uri) - .await - .map_err(|e| e.to_string())?; + let oauth_tokens = crate::auth::oauth::exchange_claude_code( + &verifier, + &expected_state, + input.trim(), + &redirect_uri, + ) + .await + .map_err(|e| e.to_string())?; crate::auth::oauth::save_claude_tokens_for_account(&oauth_tokens, label) .map_err(|e| format!("Failed to save tokens: {}", e))?; diff --git a/crates/jcode-tui/src/tui/app/auth_types.rs b/crates/jcode-tui/src/tui/app/auth_types.rs index 100fef1d12..d3957ee015 100644 --- a/crates/jcode-tui/src/tui/app/auth_types.rs +++ b/crates/jcode-tui/src/tui/app/auth_types.rs @@ -3,6 +3,8 @@ pub(crate) enum PendingLogin { /// Waiting for user to paste Claude OAuth code for a specific stored account ClaudeAccount { verifier: String, + /// Independent CSRF state (SEC-01); never the PKCE verifier. + expected_state: String, label: String, redirect_uri: Option, }, diff --git a/crates/jcode-tui/src/tui/app/tests/command_suggestions_cache.rs b/crates/jcode-tui/src/tui/app/tests/command_suggestions_cache.rs index b3d1f9a0bb..2a1698fd34 100644 --- a/crates/jcode-tui/src/tui/app/tests/command_suggestions_cache.rs +++ b/crates/jcode-tui/src/tui/app/tests/command_suggestions_cache.rs @@ -117,6 +117,7 @@ fn pending_prompt_transition_invalidates_the_memo() { // Enter a pending-login state with the same input buffer and no epoch bump. app.pending_login = Some(PendingLogin::ClaudeAccount { verifier: "test-verifier".to_string(), + expected_state: "test-state".to_string(), label: "test-account".to_string(), redirect_uri: None, }); diff --git a/crates/jcode-tui/src/tui/app/turn.rs b/crates/jcode-tui/src/tui/app/turn.rs index 412f8ff756..a61e60b795 100644 --- a/crates/jcode-tui/src/tui/app/turn.rs +++ b/crates/jcode-tui/src/tui/app/turn.rs @@ -213,11 +213,19 @@ impl App { }; status_spinner_renderer.draw_full(self, terminal)?; super::run_shell::reset_status_spinner_interval(&mut status_spinner_interval, self); - crate::network_retry::wait_until_probably_online().await; + if crate::network_retry::wait_until_probably_online() + .await + .is_online() + { + self.push_display_message(DisplayMessage::system( + "Network connectivity looks restored; retrying request.".to_string(), + )); + continue 'turn_loop; + } self.push_display_message(DisplayMessage::system( - "Network connectivity looks restored; retrying request.".to_string(), + "Still offline after waiting several minutes; giving up on this request. Check your connection and try again.".to_string(), )); - continue 'turn_loop; + return Err(err); } return Err(err); } @@ -750,11 +758,19 @@ impl App { listener: plan.listener_summary.clone(), }; status_spinner_renderer.draw_full(self, terminal)?; - crate::network_retry::wait_until_probably_online().await; + if crate::network_retry::wait_until_probably_online() + .await + .is_online() + { + self.push_display_message(DisplayMessage::system( + "Network connectivity looks restored; retrying request.".to_string(), + )); + continue 'turn_loop; + } self.push_display_message(DisplayMessage::system( - "Network connectivity looks restored; retrying request.".to_string(), + "Still offline after waiting several minutes; giving up on this request. Check your connection and try again.".to_string(), )); - continue 'turn_loop; + return Err(anyhow::anyhow!("Stream error: {}", message)); } return Err(anyhow::anyhow!("Stream error: {}", message)); } @@ -1019,11 +1035,19 @@ impl App { listener: plan.listener_summary.clone(), }; status_spinner_renderer.draw_full(self, terminal)?; - crate::network_retry::wait_until_probably_online().await; + if crate::network_retry::wait_until_probably_online() + .await + .is_online() + { + self.push_display_message(DisplayMessage::system( + "Network connectivity looks restored; retrying request.".to_string(), + )); + continue 'turn_loop; + } self.push_display_message(DisplayMessage::system( - "Network connectivity looks restored; retrying request.".to_string(), + "Still offline after waiting several minutes; giving up on this request. Check your connection and try again.".to_string(), )); - continue 'turn_loop; + return Err(e); } return Err(e); } @@ -1043,11 +1067,19 @@ impl App { listener: plan.listener_summary.clone(), }; status_spinner_renderer.draw_full(self, terminal)?; - crate::network_retry::wait_until_probably_online().await; + if crate::network_retry::wait_until_probably_online() + .await + .is_online() + { + self.push_display_message(DisplayMessage::system( + "Network connectivity looks restored; retrying request.".to_string(), + )); + continue 'turn_loop; + } self.push_display_message(DisplayMessage::system( - "Network connectivity looks restored; retrying request.".to_string(), + "Still offline after waiting several minutes; ending this turn. Check your connection and try again.".to_string(), )); - continue 'turn_loop; + break; } break; } diff --git a/crates/jcode-tui/src/tui/ui.rs b/crates/jcode-tui/src/tui/ui.rs index 30c483bf54..f980e64e81 100644 --- a/crates/jcode-tui/src/tui/ui.rs +++ b/crates/jcode-tui/src/tui/ui.rs @@ -2667,6 +2667,10 @@ pub fn draw(frame: &mut Frame, app: &dyn TuiState) { // color call sites. See `palette::adapt_buffer_for_palette` for the ordering. jcode_tui_style::adapt_buffer_for_theme(frame.buffer_mut()); jcode_tui_style::palette::adapt_buffer_for_palette(frame.buffer_mut()); + // VC-01: honor NO_COLOR/JCODE_NO_COLOR in the TUI, mirroring the CLI. Runs + // last on colors so it overrides theme/palette output with the terminal + // default; text modifiers (bold/underline) are preserved. + jcode_tui_style::palette::strip_colors_for_no_color(frame.buffer_mut()); adapt_buffer_for_emoji_preference(frame.buffer_mut()); // Cache eviction/clearing can outlive the last visible image. Carry Kitty // deletion commands on any completed frame so terminal-side pixel storage diff --git a/docs/ACCESSIBILITY.md b/docs/ACCESSIBILITY.md new file mode 100644 index 0000000000..f6b95eb978 --- /dev/null +++ b/docs/ACCESSIBILITY.md @@ -0,0 +1,70 @@ +# Accessibility + +jcode's primary interface is a character-cell terminal UI (TUI) built on +Ratatui. This document is an honest account of what that means for assistive +technology today, what already works, and what does not yet — so users relying +on screen readers or low-vision configurations can make an informed choice. + +## The short version + +- A text terminal is intrinsically more screen-reader-friendly than a + canvas/WebGL/chat-native UI: everything jcode draws is real text in the + terminal buffer, which terminal-attached screen readers can read. +- jcode is fully keyboard-driven; nothing requires a mouse. +- The known gap: jcode has **no dedicated screen-reader announcement channel**. + Streaming model tokens, evolving tool-call panels, and background-task status + are written to the cell buffer, so a screen reader only sees them if it + re-reads the region. There is no ARIA-live-region equivalent for a TUI. + +## What works today + +- **Keyboard-only operation.** All actions are reachable via key bindings; see + `docs/KEYMAP_CONFLICTS.md` for the current bindings and how to rebind. +- **Colors can be disabled.** jcode honors the `NO_COLOR` convention and its + own `JCODE_NO_COLOR`. When either is set (and for non-TTY output), colorized + CLI output is suppressed. See `src/cli/dispatch.rs` and + `src/cli/provider_doctor.rs`. +- **Status is not encoded by color alone.** The activity spinner uses distinct + Braille-pattern glyphs (`⠋⠙⠹⠸…`), and jcode automatically drops to a slower, + lower-motion "liveness" indicator in reduced-capability environments (SSH, + WSL, minimal terminals). Status therefore remains distinguishable without + color perception. +- **Theme contrast is measured.** `jcode-tui-style` includes a color-harmony + engine that scores role text/background contrast against a target and warns + on low-contrast palettes (`crates/jcode-tui-style/src/harmony.rs`). Custom + palettes that score poorly are flagged rather than shipped blindly. + +## Known limitations + +- **No announcement stream.** Off-screen buffer changes (a tool result that + scrolled up, a background task finishing) are not announced. A screen-reader + user must navigate to the region to hear updates. There is no + `--json-events`/structured-event feed yet that assistive tooling could + consume for spoken announcements (this is the tracked enhancement below). +- **Contrast is OKLab lightness-delta, not WCAG 2.x ratio**, and the shipped + theme audit is not a hard build gate. Terminal emulators also remap colors, + so the same RGB can render at different luminance; measured guarantees hold + for the computed palette, not for every emulator's rendering. +- **Fast spinner cadence.** In full-capability terminals the activity spinner + animates smoothly. There is not yet a `prefers-reduced-motion`-style opt-out + for the fast path specifically (the reduced-motion *tiers* above are driven by + terminal capability, not an explicit user preference flag). + +## Recommendations for screen-reader users + +- Set `NO_COLOR=1` (or `JCODE_NO_COLOR=1`) if your reader mis-handles ANSI color. +- Use a terminal + screen reader combination known to read cell content well + (e.g. Orca with a supported terminal on Linux). +- For scripting or bridging, prefer the harness API / SDK + (`crates/jcode-harness-api`, `crates/jcode-sdk`) which expose structured data + rather than scraping the TUI. + +## Tracked enhancement: structured event stream + +The most impactful future improvement is an opt-in machine-readable event +stream (proposed `--json-events`: JSONL on stdout or a local socket) emitting +turn start/end, tool call start/complete, and status transitions. External +assistive tooling could consume it to produce spoken announcements, and it +doubles as a scripting/automation surface. This is intentionally out of scope +for the documentation pass that added this file; it is recorded here so the +limitation is visible and the design is captured. diff --git a/docs/AUDIT_REMEDIATION.md b/docs/AUDIT_REMEDIATION.md new file mode 100644 index 0000000000..5ea92a2e67 --- /dev/null +++ b/docs/AUDIT_REMEDIATION.md @@ -0,0 +1,115 @@ +# Apodex Audit Remediation — Status & Honest Scope + +This records what each audit fix on `fix/apodex-audit-findings` actually does, +with precise scope. It exists because an earlier summary over-claimed a few +points; a follow-up review (correctly) flagged them. Each item below states the +mechanism, what it does cover, and what it deliberately does **not**. + +## P0 + +### SEC-01 — PKCE verifier no longer used as OAuth `state` (Claude) — DELIVERED +Generates an independent CSRF `state`; the PKCE `code_verifier` never enters +the authorization URL. Verified by tests asserting `state != verifier` and that +the verifier is absent from the URL. No scope caveats. + +### SEC-02 — AppleScript/JXA routed through the #604 destructive gate — DELIVERED as **heuristic defense-in-depth**, not a sandbox +`run_applescript`/`run_jxa` scripts are scanned for embedded shell payloads +(`do shell script`, JXA `doShellScript`) which are routed through the exact +shipped #604 policy, dynamic/computed shell arguments are held for +justification, and a small set of native permanent-destruction verbs +(`NSFileManager removeItem*`, `NSTask`) are flagged. + +**Scope (honest):** this is a static heuristic. It does **not** catch every +interpreter obfuscation, alternate ObjC API, or dynamic dispatch, and it is not +an OS sandbox (see SEC-05). A native-verb match with any non-empty +`justification` proceeds — that is a reflection/confirmation gate, not +prevention. Comprehensive enforcement requires OS-level sandboxing (SEC-05). + +## P1 + +### SEC-03 — SSRF guard on webfetch — DELIVERED, with connection pinning +Resolves the host and rejects if **any** resolved IP is +loopback/private/link-local (incl. `169.254.169.254`)/unspecified/multicast/ +CGNAT/IPv6-ULA/IPv4-mapped. webfetch disables auto-redirects and re-guards +**every** redirect hop, and **pins** the connection to the validated IP +(reqwest `.resolve()`), so the address checked is the address connected to — +closing the resolve-then-connect DNS-rebinding TOCTOU gap for the pinned client. + +**Scope (honest):** pinning closes the common TOCTOU case. It does **not** cover +requests that bypass the pinned client (proxies, other code paths), and there is +no allowlist escape hatch for legitimately-internal hosts yet. The `websearch` +SearXNG endpoint is intentionally **not** guarded — it is operator-configured +(config/env), commonly self-hosted on localhost/LAN, so the operator config is +the trust boundary, not a model-influenced input. + +### RC-01 — Atomic, race-free config writes — DELIVERED, intra- **and** inter-process on Unix + Windows +`save()` writes via temp-file + fsync + atomic rename (crash-safe). All ~19 +mutation helpers go through `mutate()`/`mutate_if()`, which hold both a +process-local `Mutex` **and** a cross-process advisory lock on +`config.toml.lock` across the whole load-modify-save, so two separate jcode +processes cannot lose one another's updates. The cross-process lock is +`flock(LOCK_EX)` on Unix and `LockFileEx(LOCKFILE_EXCLUSIVE_LOCK)` (released +with `UnlockFileEx`) on Windows, via the `windows-sys` dependency `jcode-base` +already carries. + +**Scope (honest):** covered on Unix and Windows — the two platforms jcode +ships on. Any other target (e.g. a hypothetical WASI build) has no advisory-lock +API wired and falls back to the in-process mutex only; that branch is explicit +in code, not silent. Acquisition is best-effort: a lock failure logs and +proceeds (never worse than before; the atomic rename still prevents a torn +file). The Windows path is API-verified against windows-sys 0.59 but is compiled +and runtime-tested only on the platform used for this work (macOS); it needs a +Windows CI leg to be runtime-verified. + +### REL-01 — Bounded network reconnect — DELIVERED +`wait_until_probably_online()` is bounded (default 300s ceiling), returns +`Online`/`GaveUp`, and all four `turn.rs` call sites surface a clear message and +stop on give-up instead of hanging forever. No scope caveats. + +## P2 + +### SEC-04 — Config file hardened to `0o600` — DELIVERED +`config.toml`, its temp file (hardened **before** secret bytes are written), the +corrupt backup, and the last-good snapshot are all owner-only, inside a `0o700` +dir. + +### REL-02 — Malformed config no longer silently resets — DELIVERED across restarts +On a parse error: the corrupt file is backed up (`config.toml.corrupt`, +`0o600`), and the config falls back to the last known-good — the in-process +snapshot if present, otherwise an on-disk `config.toml.last-good` (`0o600`, +written atomically on every good load). This survives process restarts. + +**Scope (honest):** the disk snapshot is only as fresh as the last successful +load in any prior run; if a user has never loaded a valid config on this +machine, there is nothing to recover and defaults still apply. + +## P3 / Informational + +### A11Y-01 — DELIVERED as documentation + `NO_COLOR` +`docs/ACCESSIBILITY.md` gives an honest screen-reader account. The proposed +`--json-events` structured stream is a documented tracked enhancement, not +implemented. + +### VC-01 — DELIVERED (narrow residual) +The meta-audit confirmed contrast is already computed/asserted and the CLI +honors `NO_COLOR`. The residual — the TUI renderer ignoring `NO_COLOR` — is +fixed (`strip_colors_for_no_color` in the render loop). Contrast remains OKLab +lightness-delta (not WCAG 2.x ratio) and is not a hard build gate; documented. + +### SEC-07 — DELIVERED as documentation + roadmap +`SECURITY.md` documents the install/update trust model (checksums prove +integrity, not authenticity; `curl|bash` is trust-on-first-use; binaries +unsigned) and a hardening roadmap. Signature verification itself is **not** +implemented — it requires a maintainer decision on signing-key custody. + +## Not addressed here +- **SEC-06** (iOS keychain fail-closed): Swift; not buildable on this Rust + toolchain — needs Xcode. +- **SEC-05** (OS-level command sandbox): large architectural effort; SEC-02 is + the interim heuristic mitigation. + +## Verification note +Built and tested with `cargo --ignore-rust-version` because the installed +Homebrew rustc is 1.94.0 while pinned AWS SDK crates declare MSRV 1.94.1 (a +cosmetic bump). A reviewer on stock 1.94.0 without that flag will see a +toolchain error, not a code failure. diff --git a/src/cli/login.rs b/src/cli/login.rs index c523261c3d..1978490bd6 100644 --- a/src/cli/login.rs +++ b/src/cli/login.rs @@ -76,6 +76,7 @@ enum PendingScriptableLogin { Claude { account_label: String, verifier: String, + state: String, redirect_uri: String, }, Openai { diff --git a/src/cli/login/scriptable.rs b/src/cli/login/scriptable.rs index 0133d6a5f0..a9d03c32fe 100644 --- a/src/cli/login/scriptable.rs +++ b/src/cli/login/scriptable.rs @@ -95,12 +95,15 @@ pub(super) async fn start_scriptable_login( LoginProviderTarget::Claude => { let label = auth::claude::login_target_label(account_label)?; let (verifier, challenge) = auth::oauth::generate_pkce_public(); + // SEC-01: independent CSRF state; the PKCE verifier stays client-side. + let state = auth::oauth::generate_state_public(); let redirect_uri = auth::oauth::claude::REDIRECT_URI.to_string(); - let auth_url = auth::oauth::claude_auth_url(&redirect_uri, &challenge, &verifier); + let auth_url = auth::oauth::claude_auth_url(&redirect_uri, &challenge, &state); ( PendingScriptableLogin::Claude { account_label: label, verifier, + state, redirect_uri, }, auth_url, @@ -109,6 +112,7 @@ pub(super) async fn start_scriptable_login( PendingScriptableLogin::Claude { account_label: String::new(), verifier: String::new(), + state: String::new(), redirect_uri: String::new(), } .default_expires_at_ms(), @@ -322,6 +326,7 @@ pub(super) async fn complete_scriptable_claude_login( let PendingScriptableLogin::Claude { account_label, verifier, + state, redirect_uri, } = load_pending_login(&pending_path, "claude")? else { @@ -334,7 +339,8 @@ pub(super) async fn complete_scriptable_claude_login( let selected_redirect_uri = auth::oauth::claude_redirect_uri_for_input(&raw_input, &redirect_uri); let tokens = - auth::oauth::exchange_claude_code(&verifier, &raw_input, &selected_redirect_uri).await?; + auth::oauth::exchange_claude_code(&verifier, &state, &raw_input, &selected_redirect_uri) + .await?; auth::oauth::save_claude_tokens_for_account(&tokens, &account_label)?; let profile_email = auth::oauth::update_claude_account_profile(&account_label, &tokens.access_token)