refactor: consolidate fnox client wrapper and secure stdin set#24
Closed
refactor: consolidate fnox client wrapper and secure stdin set#24
Conversation
Adds `!secure` as a post-auth chat command dispatched before any agent
sees the message. Subcommands:
- `!secure set NAME=value` — stores the secret in fnox and replies
with "Stored `NAME`" + a retention-warning. Never echoes the value.
- `!secure list` — lists stored secret names (not values). Parses
fnox's output defensively; extra columns that look like values are
stripped before replying.
- `!secure help` / `!secure` — usage string.
Dispatch model (matches existing `!switch`/`!sessions`/`!default`):
- `CommandHandler::is_secure_command(&str)` — static, case-insensitive.
- `CommandHandler::handle_secure(text, identity_id) -> String` —
async (shells out to fnox), takes identity for future per-identity
audit/ACL though not used yet. Wired into telegram.rs in both
`handle_message_nonblocking` and `handle_message` paths, and added
to the unknown-command exclusion list so `!secure` doesn't get
intercepted as unknown before hitting the post-auth handler.
Subprocess approach today — `Command::new("fnox").args(["set", …])`.
Acknowledged-cost: three places now shell out to fnox (vault.rs,
secure_set, secure_list), which means three places that need an
availability check. **Follow-up PR (in flight) will migrate all three
to the fnox library crate via `fnox = { git = "https://github.com/jdx/fnox" }`**
— fnox already exposes public modules (secret_resolver, commands,
etc.) at `src/lib.rs`, so no upstream PR is needed.
Redaction discipline:
- The success reply names the stored secret but never echoes the
value; tested.
- The failure reply surfaces fnox's stderr (user needs to know why
it failed) but doesn't carry the value; tested.
- The telegram channel's `debug!` logs only that a !secure command
was handled, without the message text. A `!secure set FOO=bar`
would otherwise leak the value into ops logs.
- Name validation: `[A-Za-z0-9_-]+` only (matches the substitution
engine's accepted ref-name syntax — so a stored secret can be
immediately referenced as `{{secret:NAME}}` without a rename).
Tested with three invalid-char cases.
5 given/when/then behavioral tests:
- `secure_set_reply_includes_name_but_not_value`
- `secure_set_surfaces_fnox_error_without_echoing_value`
- `secure_unknown_subcommand_returns_help`
- `secure_set_rejects_invalid_name_chars`
- `secure_list_returns_names_only`
All use a fake-fnox shell script installed on PATH via TempDir, so
the tests are hermetic regardless of whether the real fnox is
installed on the dev/CI machine.
Explicit non-goals for this PR (follow-ups):
- `!secure remove NAME` — pending a decision on whether fnox's
own remove command or a config-file edit is the right approach.
- `!secure request NAME` — the out-of-band localhost-paste flow
that avoids chat-transport retention entirely (see
docs/rfcs/agent-secret-gateway.md §5).
- Matrix/WhatsApp channel wiring — same pattern as telegram, but
adds separate verification surface. Doing in a follow-up to keep
this PR reviewable.
- Library-based fnox integration (swap subprocess → lib calls) —
committed as next PR per user direction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates all fnox CLI subprocess interactions behind a shared onecli_client::FnoxClient wrapper and migrates zeroclawed’s !secure set/list to use it, including passing secret values over stdin (not argv) to reduce accidental exposure.
Changes:
- Introduces
onecli_client::FnoxClient+FnoxErrorforfnox get/set/listand availability checks. - Updates
!securecommand parsing to be whitespace-aware and routes!securein the Telegram channel post-auth without logging message text. - Adds/updates tests to verify name-only responses and that secret values are not passed via argv.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
crates/zeroclawed/src/commands.rs |
Adds whitespace-aware parsing helper, !secure dispatch + set/list implementations, and test coverage for value-non-echo + stdin-only behavior. |
crates/zeroclawed/src/channels/telegram.rs |
Routes !secure as a post-auth local command and excludes it from unknown-command interception; avoids logging full text. |
crates/onecli-client/src/lib.rs |
Exposes the new fnox wrapper module and re-exports FnoxClient/FnoxError. |
crates/onecli-client/src/fnox_client.rs |
Implements consolidated subprocess wrapper for fnox operations with typed errors and unit tests. |
crates/onecli-client/Cargo.toml |
Adds tempfile as a dev-dependency for fnox wrapper tests. |
Cargo.lock |
Updates lockfile for the new dev dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+240
to
+246
| trimmed.split_whitespace().next().map(String::from) | ||
| } | ||
| }) | ||
| .collect(); | ||
| Ok(names) | ||
| } | ||
|
|
Comment on lines
+247
to
+254
| async fn run(&self, args: &[&str]) -> Result<std::process::Output, std::io::Error> { | ||
| Command::new(&self.binary) | ||
| .args(args) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .output() | ||
| .await | ||
| } |
Comment on lines
+1217
to
+1219
| if name.is_empty() || value.is_empty() { | ||
| return "⚠️ Usage: `!secure set NAME=value`".to_string(); | ||
| } |
Comment on lines
+1965
to
+2005
| static SECURE_ENV_MUTEX: Mutex<()> = Mutex::new(()); | ||
|
|
||
| fn install_fake_fnox(dir: &TempDir, body: &str) -> std::path::PathBuf { | ||
| let bin = dir.path().join("fnox"); | ||
| fs::write(&bin, format!("#!/bin/sh\n{body}\n")).expect("write fake fnox"); | ||
| let mut perms = fs::metadata(&bin).unwrap().permissions(); | ||
| perms.set_mode(0o755); | ||
| fs::set_permissions(&bin, perms).expect("chmod fake fnox"); | ||
| dir.path().to_path_buf() | ||
| } | ||
|
|
||
| struct PathGuard { | ||
| original: Option<String>, | ||
| } | ||
| impl PathGuard { | ||
| fn prepend(dir: &std::path::Path) -> Self { | ||
| let original = std::env::var("PATH").ok(); | ||
| let new_path = match &original { | ||
| Some(p) => format!("{}:{}", dir.display(), p), | ||
| None => dir.display().to_string(), | ||
| }; | ||
| // Safety: tests holding SECURE_ENV_MUTEX serialize env | ||
| // mutation. `std::env::set_var` is marked unsafe in | ||
| // Rust 2024 for this exact reason. | ||
| unsafe { | ||
| std::env::set_var("PATH", new_path); | ||
| } | ||
| Self { original } | ||
| } | ||
| } | ||
| impl Drop for PathGuard { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| match &self.original { | ||
| Some(p) => std::env::set_var("PATH", p), | ||
| None => std::env::remove_var("PATH"), | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
Owner
Author
|
Codex integration sweep note: I reviewed the inline comments on this PR. GitHub rejected direct inline replies for these older/outdated review comments with HTTP 422, so I am responding top-level instead: 3141257308, 3141257326, 3141257333, 3141257340.\n\nI did not edit this branch. Items that overlap the secure/fnox/host-agent/digest integration work are addressed in draft PR #38 (codex-integration-code), including stdin-based fnox set, bounded fnox waits, whitespace-safe !secure parsing, identity-aware !secure audit logs, valid-input host-agent properties, real WhatsApp HMAC verification, loopback OneCLI default bind, and race-free digest temp paths. Remaining PR-specific findings stay actionable for this branch owner or a follow-up. |
Owner
Author
|
Acknowledged. The non-codex |
Owner
Author
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
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.
Summary
Draft follow-up that folds the useful
refactor/fnox-client-wrapperwork into an owned Codex branch without modifying the parallelfeat/secure-chat-commandsPR branch.onecli_client::FnoxClientas the shared subprocess wrapper forfnox get,fnox set,fnox list, and availability checks.!secure set/listonto the wrapper.fnox setto pass secret values over stdin instead of argv, keeping values out ofps/process-monitor output.!secureparsing whitespace-aware for pasted tab/newline input.secure.setandsecure.listwith authenticated identity and secret name only.Stack / Coordination
This branch includes the
feat/secure-chat-commandscommit (a4a09ffe) plus one Codex-owned follow-up commit. Review after/alongside PR #20. It does not update or depend on pushing to the other agent's branch.Tests
cargo test -p onecli-client fnox_clientcargo test -p zeroclawed secure_cargo clippy -p onecli-client -p zeroclawed --all-targets -- -D warningsNotes
vault.rs::get_secret_from_fnoxstill has its own subprocess path on branches where the fnox vault fallback exists; that should be the next consolidation target once the relevant stack is settled.