[temp] write-permission probe - #1
Closed
ra-co88 wants to merge 15 commits into
Closed
Conversation
The Claude login flow passed the PKCE `code_verifier` as the OAuth `state` parameter, embedding a client secret verbatim in the authorization URL (browser history, provider access logs, QR/manual-paste, terminal scrollback). Anyone observing the URL obtained the verifier, defeating the core guarantee of PKCE S256. Generate an independent `state` via `generate_state()` and thread an `expected_state` through `login_claude`, `exchange_claude_code[_at_url]`, the TUI login path, and the scriptable CLI. The verifier now stays client-side, only ever sent in the TLS-protected token-exchange body. Regression test: assert the authorization URL's `state != verifier` and that the verifier never appears in the URL (replaces the prior test that pinned the buggy `state == verifier` behavior).
…ive gate (SEC-02) `macos_computer_use`'s run_applescript/run_jxa executed model-supplied scripts via osascript with no destructive-command gate, while the bash tool's 1jehuang#604 gate held identical `rm -rf $HOME` payloads. A prompt-injected or mistaken agent could destroy files or exfiltrate secrets through the scripting actions, re-creating the 1jehuang#604 data-loss class via a neighboring tool. Promote the gate from bash-private to a shared `tool::destructive_gate` module (rename bash_destructive_gate.rs -> destructive_gate.rs, declared in tool/mod.rs; functions widened to pub(crate); add a caller-labeled variant). Add `applescript_destructive_refusal`: it extracts embedded shell payloads (`do shell script`, JXA `doShellScript`) and routes literals through the exact shipped 1jehuang#604 policy, holds computed/dynamic shell arguments for justification, and flags native permanent-destruction verbs (NSFileManager removeItem*, NSTask); Finder Trash is intentionally allowed. Wire it into the run_applescript/run_jxa dispatch arms, thread working_dir through execute->run->dispatch, and add a `justification` field so a refused script can be re-issued like bash. Honest defense-in-depth (cf. SEC-05): a static scan cannot catch every interpreter obfuscation; catastrophic targets stay blocked even with justification. 8 unit tests.
webfetch validated only the URL scheme; it had no allow/blocklist for private ranges, and the shared HTTP client followed redirects by default. An agent could be steered to read cloud instance metadata (169.254.169.254), scan the LAN, or hit loopback dev services — and a public URL could 30x-redirect into that space after any pre-flight check. Add a shared `tool::ssrf` guard: parse+require http(s), reject localhost/.local, resolve the host and refuse if ANY resolved IP is loopback/private/link-local (incl. 169.254.169.254 metadata)/unspecified/ broadcast/multicast/CGNAT/IETF-reserved, plus IPv6 ULA/link-local and IPv4-mapped forms (defeats DNS-rebinding via a single private A record). webfetch now uses a no-auto-redirect client and follows redirects manually (bounded, MAX_REDIRECTS=5), re-running the guard on every hop. Scope: the strict guard applies to the model-supplied webfetch URL. The websearch SearXNG endpoint is left unguarded because it is operator- configured (config/env) and commonly self-hosted on localhost/LAN — the operator config is the trust boundary, not a model-influenced input. 3 tests.
Config mutation helpers did `load -> mutate -> save` with no locking (RC-01: two concurrent writers to disjoint fields silently lost one update), and `save()` used a bare `std::fs::write` with default perms (SEC-04: config.toml can hold provider api_keys yet was world-readable on inherited/shared directories, and a crash mid-write could truncate it). RC-01: add a process-wide CONFIG_WRITE_LOCK and a `Config::mutate()` / `mutate_if()` that hold the lock across the WHOLE load->modify->save cycle; route all ~19 `set_*` and external-auth-trust helpers through them. (Locking only the write was insufficient — the added concurrency test caught that disjoint writes still raced.) SEC-04 + durability: `save()` now writes to a temp file in the same directory, hardens it to 0o600 before the secret bytes land, fsyncs, then atomically renames over the target, and hardens the file+dir via `jcode-storage::harden_secret_file_permissions` (0o600/0o700, Windows-aware). Tests: owner-only permissions after save, and 8-thread concurrent disjoint writes preserve both updates and leave the file parseable.
…REL-01)
`wait_until_probably_online()` looped forever with exponential backoff and
no ceiling. A permanent offline state (broken VPN profile, captive portal
that never satisfies the probe) wedged all four turn.rs recovery call sites
indefinitely, with a spinner but no elapsed bound and no escape.
Bound it: the function now returns `ReconnectOutcome { Online, GaveUp }` and
delegates to `wait_until_probably_online_bounded(max_total)` with a default
300s ceiling; the loop checks elapsed time each iteration and never sleeps
past the remaining budget. All four turn.rs call sites now check
`.is_online()` — on GaveUp they surface a clear "still offline after
waiting several minutes" message and stop (return/break) instead of looping.
Test asserts a bounded wait terminates within its ceiling regardless of
network state.
Ignore `.agents/` (Orion-OS local memory bank / working context, per its ops manual — travels with the machine, not the repo), alongside the pre-existing `.personal/`, and `.DS_Store` (macOS Finder metadata).
…tting (REL-02) A single TOML syntax error (a hand edit or a model config-edit slip) made `load_from_file()` log and return None, so `load()` fell through to `Config::default()` — silently wiping every user setting, including security opt-outs like telemetry/discovery, on the next reload. Now, on a parse/read error, `load_from_file()` (1) backs up the corrupt file to `config.toml.corrupt` (owner-only, idempotent per corrupt version) so it is recoverable and never silently overwritten by the next save, and (2) returns the last config that parsed successfully in this process (new LAST_GOOD_CONFIG snapshot) instead of defaults. Interactive callers still surface the error via the existing `load_strict()` path (config_edit_notice). Test: after corrupting the file, load() keeps the prior centered=true and writes a .toml.corrupt backup containing the bad bytes.
…Y-01, VC-01) A11Y-01: add docs/ACCESSIBILITY.md — an honest account of the TUI's screen-reader story: what works (keyboard-only, NO_COLOR, glyph-based status, measured theme contrast), the known gap (no announcement channel / no --json-events stream yet), and the OKLab-vs-WCAG contrast caveat. Linked from the README UI section. VC-01 (narrow residual): the meta-audit confirmed contrast IS already computed and asserted (jcode-tui-style/harmony.rs) and NO_COLOR IS honored in the CLI — but the TUI renderer ignored it. Add `palette::strip_colors_for_no_color()` and call it once per frame in the render loop (ui.rs) so NO_COLOR/JCODE_NO_COLOR drops all fg/bg/underline to the terminal default while preserving text modifiers, matching the CLI. Kept as a buffer-level pass at the existing palette chokepoint rather than adding a ColorCapability variant (which would ripple across two crates). Tests: NO_COLOR strips every cell to Reset; disabled leaves colors intact.
Add SECURITY.md: an honest account of what the installer and in-app updater verify (SHA-256 checksums over HTTPS, multi-source), the residual risk (checksums share a trust root with the binary, so they prove integrity not authenticity; curl|bash is trust-on-first-use; binaries are unsigned), cautious-install guidance, and a concrete hardening roadmap (detached SHA256SUMS signatures verified in jcode-update-core, multi-channel publication, platform notarization, pinned installer artifact hash). Signature verification itself requires a maintainer decision on signing-key custody, so it is documented as a tracked roadmap item rather than half-implemented. Linked from the README install section. Informational finding SEC-07.
Address three gaps found reviewing the first pass: RC-01 (audit #1): the write lock was process-local only, so two separate jcode processes could still lose updates. Add a cross-process advisory lock (ConfigFileLock: flock(LOCK_EX) on a 0o600 config.toml.lock on Unix; a documented no-op elsewhere where only the in-process mutex applies) held across the whole read-modify-write in mutate_if. REL-02 (audit #2): last-good was in-process only, so a fresh process with a corrupt config.toml still fell back to Config::default(). Persist a byte-for-byte last-good snapshot to config.toml.last-good on every good load (atomic, 0o600) and restore from it when the in-process snapshot is empty, so known-good settings survive restarts. SEC-04 (audit #3): back_up_corrupt_config used std::fs::write then hardened afterward — a window where key-bearing bytes sat at default perms. Route it through the existing write_atomic_hardened (temp created 0o600 before the secret bytes, then atomic rename). Tests: extend the malformed-config test to assert across-restart recovery from disk and 0o600 on both the corrupt backup and the last-good snapshot; add an inter-process lock-file test (created, reusable, owner-only).
…inding (SEC-03 #4) The prior SSRF guard resolved+checked the host, but reqwest re-resolved at connect time — a hostile DNS could return a public IP during validation and an internal IP during connection (TOCTOU). The code documented this gap; the audit follow-up asked to close it. Add `guard_public_url_pinned` which returns the validated `SocketAddr`, and in webfetch build a per-hop no-redirect client with reqwest `.resolve(host, addr)` so the connection uses exactly the IP that was validated. Applied on every redirect hop, so both the initial fetch and any 30x land on a checked address. Literal-IP URLs need no pinning (no name to re-resolve) and return None. `guard_public_url` is kept as a thin wrapper. Updated the module doc to state the TOCTOU gap is now closed for the pinned client (residual: proxies / non-pinned code paths, and still no internal-host allowlist). Test: pinned guard rejects internal targets and returns no pin for a literal public IP; existing webfetch tests still pass.
A follow-up review correctly flagged that an earlier summary over-claimed a few fixes. Add docs/AUDIT_REMEDIATION.md stating, per finding, the exact mechanism and its scope — including the deliberately-scoped ones: - SEC-02 is heuristic defense-in-depth, not a scripting sandbox; a justified native-verb call is a reflection gate, not prevention (full enforcement = SEC-05 OS sandbox). - SEC-03 pinning closes the common DNS-rebinding TOCTOU but not proxy/non- pinned paths; SearXNG is intentionally operator-trusted. - RC-01 cross-process lock is Unix-only (in-process mutex only elsewhere). - REL-02 recovers last-good across restarts via an on-disk snapshot, bounded by whether a valid config was ever loaded on the machine. - SEC-07 and A11Y-01 `--json-events` are documented roadmap, not implemented. - Verification uses `--ignore-rust-version` (1.94.0 vs cosmetic 1.94.1 MSRV). This is the authoritative, accurately-scoped record addressing the review's recommendation to correct the summary.
The inter-process config write lock was Unix-only (flock); on Windows two
jcode processes could still lose one another's config updates. Wire the
Windows path so RC-01 is closed on both platforms jcode ships on.
- ConfigFileLock now holds the lock file on Unix and Windows. Acquire uses
flock(LOCK_EX) on Unix and LockFileEx(LOCKFILE_EXCLUSIVE_LOCK) on Windows;
Drop releases via flock(LOCK_UN) / UnlockFileEx. Non-unix/non-windows
targets keep the explicit in-process-mutex-only fallback.
- Uses windows-sys (already a jcode-base Windows dep); adds the
Win32_Storage_FileSystem + Win32_System_IO features. Cargo.lock unchanged.
- The lock-file open/harden is refactored into open_lock_file() shared by
both platforms.
Docs: docs/AUDIT_REMEDIATION.md RC-01 section updated to state the lock is
now cross-platform (Unix + Windows), with the honest caveat that the Windows
path is API-verified against windows-sys 0.59 but runtime-tested only on
macOS (needs a Windows CI leg).
Changelog: add changelog/v0.81.0.json ("Security & reliability hardening")
summarizing the user-visible effect of the audit fixes (SEC-01/02/03/04/07,
RC-01, REL-01/02, A11Y-01, VC-01) and index it.
Verified: config:: tests still pass on macOS (4/4 in atomic_save_tests,
including the inter-process lock-file test); config_file.rs compiles clean.
ra-co88
pushed a commit
that referenced
this pull request
Aug 30, 2026
Completes the client side of verify-then-commit (docs/VERIFY_THEN_COMMIT.md gap #1): the remote TUI can now answer interactive stdin requests instead of only noticing them. - ServerEvent::StdinRequest arms a pending-answer state and shows the prompt in the transcript (empty prompts get a status notice) - Enter sends the composer line as Request::StdinResponse - the first caller of the previously dead send_stdin_response path - so verify- then-commit approvals and interactive bash stdin both work remotely - Esc declines with an empty reply (the edit gate treats it as a rejection) while preserving the draft for reuse - other keys fall through so the user can scroll/edit while waiting - replies are excluded from cross-session prompt history - 4 wire-level tests using the dummy socketpair idiom assert the actual StdinResponse frames; in-workspace edit_approval suite now 13/13 Default remains off: local sessions have no stdin channel today, so the gate fails closed there (documented).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test