refactor: FnoxClient wrapper — consolidates fnox subprocess call-sites#21
Closed
refactor: FnoxClient wrapper — consolidates fnox subprocess call-sites#21
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>
User and I had a back-and-forth on whether to migrate fnox usage to
the upstream Rust library or keep subprocess. After investigating
fnox's actual library API (`fnox::secret_resolver::resolve_secret`
needs pre-loaded `Config` + per-secret `SecretConfig`; no clean
programmatic SET; ~30 transitive deps), subprocess is still the
right tool — but each call-site shouldn't be reinventing the wheel.
This commit lands `crates/onecli-client/src/fnox_client.rs` — one
typed wrapper around `fnox` that all current and future call-sites
use:
- `FnoxClient::get(name) -> Result<String, FnoxError>`
- `FnoxClient::set(name, value) -> Result<(), FnoxError>`
- `FnoxClient::list() -> Result<Vec<String>, FnoxError>`
- `FnoxClient::is_available() -> bool`
Errors are typed (`NotInstalled`, `EmptyValue`, `Failed { exit_code,
stderr }`, `InvalidUtf8`), not stringified-stderr, so callers can
pattern-match: e.g. `!secure set` gives a different message on
NotInstalled (suggest `brew install fnox`) vs Failed (surface fnox's
own error).
Migration in this PR:
- `commands.rs::secure_set` and `secure_list` now call FnoxClient.
Before: each had its own `Command::new("fnox")`, its own io::Error
-> "not available" stringification, its own stderr-to-string error.
After: one-liner calls + match on the typed error.
Migration left for follow-up (when PR #15 lands or this branch is
rebased on it):
- `vault.rs::get_secret_from_fnox` — same swap; needs PR #15 in
history first since that's where the function lives.
Tests:
- 11 new given/when/then tests for FnoxClient itself, using
`FnoxClient::with_binary(path)` to point at fake-fnox shell scripts
in TempDir. No PATH manipulation, no global env mutation, no
`serial_test` requirement (cleaner than the existing fake-fnox-on-
PATH pattern in `vault_fallthrough.rs` and `commands::tests`).
- Existing 5 `!secure` tests in `commands::tests` still pass
unchanged — they used PATH-prepend, which still works because
`FnoxClient::new()` defaults to `"fnox"` (PATH lookup).
- Notable adversarial assertions added:
- `get_empty_value_is_error_not_empty_string` — empty stdout
returned as `EmptyValue(name)` so a downstream `Authorization:
Bearer ` (empty) silent-anonymous failure is impossible.
- `set_passes_name_and_value_in_argv_order` — captures argv to a
temp file and asserts position; guards against an off-by-one
rearrangement that would store the value under the wrong name.
- `list_extracts_names_only_dropping_value_columns` — explicit
negative assertion that no value substring survives.
`tempfile` added as a dev-dep on `onecli-client`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a typed FnoxClient wrapper in onecli-client to centralize all fnox CLI subprocess interactions, then migrates ZeroClawed’s !secure set / !secure list command implementations to use that wrapper.
Changes:
- Add
onecli_client::FnoxClient+FnoxError(get/set/list/is_available) with hermetic fake-binary tests. - Refactor
!securehandling inzeroclawedto callFnoxClientinstead of bespokeCommand::new("fnox")blocks. - Update Telegram channel routing to recognize
!secureas a post-auth command and avoid logging the raw command text.
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 !secure detection/handler and migrates secure_set/secure_list to FnoxClient; adds !secure tests. |
| crates/zeroclawed/src/channels/telegram.rs | Wires !secure handling into Telegram (both blocking and nonblocking paths) and excludes it from unknown-command routing. |
| crates/onecli-client/src/lib.rs | Exposes the new fnox_client module and re-exports FnoxClient/FnoxError. |
| crates/onecli-client/src/fnox_client.rs | Implements the FnoxClient wrapper, error types, and unit tests via fake binaries. |
| crates/onecli-client/Cargo.toml | Adds tempfile dev-dependency for FnoxClient tests. |
| Cargo.lock | Locks the added tempfile dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+876
to
+880
| // `!secure ...` — split off the subcommand word. | ||
| let mut parts = trimmed.splitn(3, ' '); | ||
| let _lead = parts.next(); // `!secure` | ||
| let sub = parts.next().map(|s| s.to_lowercase()).unwrap_or_default(); | ||
| let rest = parts.next().unwrap_or("").trim(); |
There was a problem hiding this comment.
handle_secure parses the subcommand via splitn(3, ' '), which breaks on multiple spaces (e.g. "!secure set ..." yields an empty sub and falls back to help). Consider parsing by stripping the !secure prefix, trim_start, then extracting the next word while tolerating repeated whitespace so set/list/help dispatch remains reliable.
Suggested change
| // `!secure ...` — split off the subcommand word. | |
| let mut parts = trimmed.splitn(3, ' '); | |
| let _lead = parts.next(); // `!secure` | |
| let sub = parts.next().map(|s| s.to_lowercase()).unwrap_or_default(); | |
| let rest = parts.next().unwrap_or("").trim(); | |
| // `!secure ...` — strip the command prefix, then extract the next word | |
| // while tolerating repeated whitespace between tokens. | |
| let args = trimmed | |
| .strip_prefix("!secure") | |
| .unwrap_or(trimmed) | |
| .trim_start(); | |
| let (sub, rest) = if let Some(idx) = args.find(char::is_whitespace) { | |
| let (sub, rest) = args.split_at(idx); | |
| (sub.to_lowercase(), rest.trim_start()) | |
| } else { | |
| (args.to_lowercase(), "") | |
| }; |
Comment on lines
+210
to
+222
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| let names = stdout | ||
| .lines() | ||
| .filter_map(|line| { | ||
| let trimmed = line.trim(); | ||
| if trimmed.is_empty() || trimmed.starts_with('#') { | ||
| None | ||
| } else { | ||
| trimmed.split_whitespace().next().map(String::from) | ||
| } | ||
| }) | ||
| .collect(); | ||
| Ok(names) |
There was a problem hiding this comment.
The FnoxError::InvalidUtf8 variant implies callers can rely on a hard error for non‑UTF8 output, but list() currently uses String::from_utf8_lossy(&output.stdout) so invalid bytes will be silently replaced and the function will still return Ok(names). It would be more consistent to decode with String::from_utf8(output.stdout) (and return InvalidUtf8 on error), and add a test that asserts invalid stdout triggers InvalidUtf8.
Comment on lines
+6
to
+15
| //! Three call-sites in this workspace shell out to `fnox`: | ||
| //! | ||
| //! - `vault.rs::get_secret_from_fnox` (read path, hot) | ||
| //! - `commands.rs::secure_set` (write path, `!secure set`) | ||
| //! - `commands.rs::secure_list` (enumerate names, `!secure list`) | ||
| //! | ||
| //! Pre-this-module each call-site had its own `Command::new("fnox")`, | ||
| //! its own stderr-to-string error, and its own | ||
| //! "is-fnox-installed?" check. That's three near-identical | ||
| //! implementations, three subtly-different error messages, and three |
There was a problem hiding this comment.
Module docs claim there are three fnox subprocess call-sites and reference vault.rs::get_secret_from_fnox, but that function/call-site doesn't exist in the current tree. Please update this doc block to reflect the actual call-sites (or point at the correct vault function) so future readers aren't sent on a dead hunt.
Suggested change
| //! Three call-sites in this workspace shell out to `fnox`: | |
| //! | |
| //! - `vault.rs::get_secret_from_fnox` (read path, hot) | |
| //! - `commands.rs::secure_set` (write path, `!secure set`) | |
| //! - `commands.rs::secure_list` (enumerate names, `!secure list`) | |
| //! | |
| //! Pre-this-module each call-site had its own `Command::new("fnox")`, | |
| //! its own stderr-to-string error, and its own | |
| //! "is-fnox-installed?" check. That's three near-identical | |
| //! implementations, three subtly-different error messages, and three | |
| //! Multiple call-sites in this workspace shell out to `fnox`, | |
| //! including the vault read path and the `!secure set` / `!secure | |
| //! list` command handlers. | |
| //! | |
| //! Pre-this-module each call-site had its own `Command::new("fnox")`, | |
| //! its own stderr-to-string error, and its own | |
| //! "is-fnox-installed?" check. That meant near-identical | |
| //! implementations, subtly-different error messages, and multiple |
Comment on lines
+1939
to
+1943
| // These use the same fake-fnox-on-PATH trick as | ||
| // `onecli-client/tests/vault_fallthrough.rs`: a temp dir holding a | ||
| // shell script named `fnox` goes to the FRONT of PATH; that script | ||
| // acts like fnox for the test's purposes. Real fnox presence on | ||
| // the dev machine doesn't affect the result. |
There was a problem hiding this comment.
This test module comment references onecli-client/tests/vault_fallthrough.rs, but there is no crates/onecli-client/tests directory (and no vault_fallthrough test) in the current repo. Please update/remove the reference so the comment accurately describes the provenance of the PATH-based fake-fnox technique.
Suggested change
| // These use the same fake-fnox-on-PATH trick as | |
| // `onecli-client/tests/vault_fallthrough.rs`: a temp dir holding a | |
| // shell script named `fnox` goes to the FRONT of PATH; that script | |
| // acts like fnox for the test's purposes. Real fnox presence on | |
| // the dev machine doesn't affect the result. | |
| // These tests use a fake-fnox-on-PATH setup: a temp dir holding a | |
| // shell script named `fnox` goes to the front of PATH, and that | |
| // script behaves like fnox for the test's purposes. Any real fnox | |
| // installed on the dev machine does not affect the result. |
This was referenced Apr 25, 2026
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: 3141204078, 3141204095, 3141204100, 3141204110.\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. |
bglusman
added a commit
that referenced
this pull request
Apr 25, 2026
Cross-cutting triage finding (PRs #20, #21, #23, #26, #28, #31): the !secure parser used `splitn(' ')` which mis-shapes multi-space input — `"!secure set NAME=v"` parsed as sub="" rest="set NAME=v", silently routing through the unknown-subcommand path with the secret in the help-error message. Switch to `split_whitespace()`. Also wires the previously-unused `identity_id` into an audit log so the `_` prefix can drop. The log records identity + subcommand, never the value (`rest`). Closes the "audit-claim docstring with no audit implementation" finding from triage.
This was referenced Apr 25, 2026
bglusman
added a commit
that referenced
this pull request
Apr 25, 2026
…ning (#44) Squash-merge of integration/super-combined — 4 weeks of feature work + cross-PR security fixes + codex agent's hardening, all green CI (14/14 checks). ## Features landing - **fnox secret-resolver integration** (#15) + FnoxClient subprocess wrapper (#21) - **Adversarial commit-reviewer + mechanical pre-commit gate** (#18) - **{{secret:NAME}} substitution engine** in security-proxy URL/headers/body (#19) - **Per-secret destination allowlist** (#22) — RFC §11.1 attack defense - **!secure chat commands** (set/list) on Telegram (#20), Matrix (#28), WhatsApp (#31) - **zeroclawed-mcp** scaffold — agent-facing secret discovery server (#23) - **install.sh wires MCP** into Claude Code agent configs (#26) - **zeroclawed-secret-paste** — localhost web UI for one-shot secret input (#34) - **Bulk paste UI** — .env-style multi-secret onboarding with per-line results - **LAN-friendly defaults** — bind 0.0.0.0 + RFC 1918 Origin acceptance - **WhatsApp HMAC verification** (was always-true placeholder before — codex hardening) ## Security fixes folded in - /vault/:secret bearer auth + 127.0.0.1 default bind (#39) - URL-embedded secrets honor destination allowlist (#41) - Paste-flow: bearer URL only at debug, fnox set via stdin not argv (#40) - Paste-flow: graceful shutdown, exit-on-submit, reject Origin: null (#43) - Subprocess timeouts + kill_on_drop on FnoxClient - BrokenPipe-tolerant stdin write (Linux CI surface) - Header-value log redaction - OneCLI bound to 127.0.0.1 by default - Sanitized real API token + Telegram IDs from sample configs (#36) ## Architecture / refactors - Consolidated onecli binary into security-proxy (#17) - Hardcoded vault URL removed from onecli-client - security-proxy resolver wired into hot path - Extracted build_app router; migrated /vault/:secret route - !secure parser uses split_whitespace (was splitn), audit-logs invocations ## Test coverage added - security-proxy substitution engine + body/headers tests - onecli-client retry + Http(_) variant + adversarial fallthrough suite - onecli-client client.rs rewritten from tautologies to wiremock-backed - config/validator coverage (was zero, now 290-line module covered) - 16 zeroclawed-secret-paste tests including bulk-mode cases ## Docs / RFCs - agent-secret-gateway holistic architecture - consolidation-findings (what #28 must address) - secret-input-web-ui RFC (input-only, new-by-default) - browser-harness integration spike - test-quality-audit Round 1+2+3 (host-agent + zeroclawed priority files) ## Codex agent's hardening cherry-picks - Subprocess timeouts on fnox calls - map_spawn_error helper - Validator hardening + atomic-counter digest race fix - WhatsApp HMAC implementation + tests - proxy header-value log redaction CI: all 14 checks green at squash time. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Owner
Author
|
Subsumed by #44 (squashed to |
bglusman
added a commit
that referenced
this pull request
Apr 25, 2026
…ning (#44) Squash-merge of integration/super-combined — 4 weeks of feature work + cross-PR security fixes + codex agent's hardening, all green CI (14/14 checks). ## Features landing - **fnox secret-resolver integration** (#15) + FnoxClient subprocess wrapper (#21) - **Adversarial commit-reviewer + mechanical pre-commit gate** (#18) - **{{secret:NAME}} substitution engine** in security-proxy URL/headers/body (#19) - **Per-secret destination allowlist** (#22) — RFC §11.1 attack defense - **!secure chat commands** (set/list) on Telegram (#20), Matrix (#28), WhatsApp (#31) - **zeroclawed-mcp** scaffold — agent-facing secret discovery server (#23) - **install.sh wires MCP** into Claude Code agent configs (#26) - **zeroclawed-secret-paste** — localhost web UI for one-shot secret input (#34) - **Bulk paste UI** — .env-style multi-secret onboarding with per-line results - **LAN-friendly defaults** — bind 0.0.0.0 + RFC 1918 Origin acceptance - **WhatsApp HMAC verification** (was always-true placeholder before — codex hardening) ## Security fixes folded in - /vault/:secret bearer auth + 127.0.0.1 default bind (#39) - URL-embedded secrets honor destination allowlist (#41) - Paste-flow: bearer URL only at debug, fnox set via stdin not argv (#40) - Paste-flow: graceful shutdown, exit-on-submit, reject Origin: null (#43) - Subprocess timeouts + kill_on_drop on FnoxClient - BrokenPipe-tolerant stdin write (Linux CI surface) - Header-value log redaction - OneCLI bound to 127.0.0.1 by default - Sanitized real API token + Telegram IDs from sample configs (#36) ## Architecture / refactors - Consolidated onecli binary into security-proxy (#17) - Hardcoded vault URL removed from onecli-client - security-proxy resolver wired into hot path - Extracted build_app router; migrated /vault/:secret route - !secure parser uses split_whitespace (was splitn), audit-logs invocations ## Test coverage added - security-proxy substitution engine + body/headers tests - onecli-client retry + Http(_) variant + adversarial fallthrough suite - onecli-client client.rs rewritten from tautologies to wiremock-backed - config/validator coverage (was zero, now 290-line module covered) - 16 zeroclawed-secret-paste tests including bulk-mode cases ## Docs / RFCs - agent-secret-gateway holistic architecture - consolidation-findings (what #28 must address) - secret-input-web-ui RFC (input-only, new-by-default) - browser-harness integration spike - test-quality-audit Round 1+2+3 (host-agent + zeroclawed priority files) ## Codex agent's hardening cherry-picks - Subprocess timeouts on fnox calls - map_spawn_error helper - Validator hardening + atomic-counter digest race fix - WhatsApp HMAC implementation + tests - proxy header-value log redaction CI: all 14 checks green at squash time. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
Resolves the open question raised in PR #20's discussion thread:
should we migrate fnox usage to the upstream Rust library, or stay
subprocess?
After investigating fnox's library API, subprocess is still the right
tool for our use case — but each call-site shouldn't reinvent the
wheel. This PR lands a typed wrapper (`FnoxClient`) that all current
and future call-sites use.
Why not the library
`fnox::secret_resolver::resolve_secret` requires pre-loaded
`Config` + per-secret `SecretConfig` (CLI-style "I'm building
another fnox", not "I want a programmatic get"). No clean
programmatic SET. ~30 transitive deps (AWS SDK, GCP SDK, keyring)
that we wouldn't use. Subprocess is the right tool today; if fnox
adds a clean programmatic API later, swap the implementation behind
`FnoxClient` without touching callers.
What's in this PR
`get`, `set`, `list`, `is_available`. Typed
`FnoxError` (`NotInstalled` / `EmptyValue` / `Failed` /
`InvalidUtf8`) so callers can pattern-match instead of
substring-checking stderr.
use it. Each call-site went from a 30-line `Command::new`
block to a one-liner.
`FnoxClient::with_binary(path)` to point at fake-fnox shell
scripts in `TempDir`. No PATH manipulation, no global env
mutation needed.
Notable adversarial assertions:
returned as `EmptyValue(name)` so a downstream
`Authorization: Bearer ` (empty) silent-anonymous failure is
impossible.
a temp file and asserts position; guards against off-by-one
rearrangements that would store the value under the wrong name.
Migration left for follow-up
`vault.rs::get_secret_from_fnox` should also use FnoxClient.
That code lives on PR #15's branch (`feat/fnox-integration`); the
swap is one function-body replacement after either:
I've left a comment in the FnoxClient doc explicitly listing
vault.rs as a known follow-up call-site.
Stacks on PR #20
`feat/secure-chat-commands` adds the `secure_set` and
`secure_list` functions this PR migrates. Review #20 first.
🤖 Generated with Claude Code