Repository navigation
fix(tui): re-read the network policy so /network allow lands without a restart - #8
SparkofSpike wants to merge 4 commits into
Conversation
…a restart `/network allow <host>` edits config.toml and prints "Retry the command now.", but the engine held the NetworkPolicyDecider captured when the session was spawned, so the retry was refused for the rest of the session and the host only unblocked after a restart. The per-turn tool context and the MCP pool now take a decider rebuilt from the document's own [network] table. The session cache rides along, so an approval already granted through the approval prompt survives the refresh. A session launched without a [network] table stays ungated on purpose: adopting one from disk would hand that session a default = "prompt" policy it never had, and every unrelated host would start prompting. This follows the WorkspaceTrust precedent, which also re-reads per tool-context build so /trust add lands mid-session. As a side effect a hand edit of [network] in config.toml now takes effect on the next turn too. Known limitation: [profiles.<name>.network] is not consulted, matching what /network writes.
|
Adversarial review of #8 — Verdict: The fix genuinely repairs the primary reported path (the per-turn bash-gate re-reads Findings, most severe first. Finding 1 — CONFIRMED (security, fail-open): the re-read silently replaces the live policy, dropping any stricter layer — including a managed overrides
But the spawn-time snapshot it replaces is not just the base document. The TUI's effective On the first turn after The failure matrix (which I traced in
So the author's claim that "the caller keeps the policy that it already holds, which is the conservative direction" (config.rs:3068-3071) holds only for the Consequence: In an org forbidding Finding 2 — CONFIRMED: the MCP path is over-claimed; multi-session /network allow never reaches the pool
Once a pool exists ( So: the MCP hosts that were blocked with Finding 3 — CONFIRMED: "stays ungated" is fail in the tightening (deny) direction
A session that began with no Finding 4 (CONFIRMED, the actual primary path) — the fix does land for the reported bash-tool gateTo be fair and precise: statically the primary path does hold. The per-turn tool registry is built from Suspected caveat on this same path: checkpoints/ Finding 5 — CONFIRMED: the integration test only proves
|
The first cut re-read `[network]` from the user's document and treated the result as the live policy. That document is only one layer of it: a managed overlay replaces the user table, and a Fleet `network_access = false` never consults it at all — `exec_network_policy` synthesizes a `default = "deny"` decider for exactly that case. Replacing the resolved policy with the raw document therefore widened an administrator's denial to prompt/allow on the first tool-context build, which is every turn. `Config` now carries a runtime-only receipt that a managed overlay supplied `[network]`, and the Fleet-synthesized decider is marked authoritative. When either holds, a mid-session re-read is folded onto the resolved policy instead of replacing it: the document's `allow` list is adopted — `allow <host>` still works — but the fallback keeps the stricter of the two decisions and a denial is never dropped. This also lands the deny direction: a session that started with no policy at all adopts the table the moment one exists, so `/network deny <host>` is not ignored in the case where a policy is being tightened. Tests: `folding_a_document_never_widens_an_authority` covers the fold (deny kept, fallback unchanged, allow adopted), `refreshed_policy_keeps_the_ authority_marking` covers the marker surviving a refresh, and `tool_context_adopts_a_policy_written_mid_session_and_never_ungates_one` covers adoption plus the reverse direction (removing the table leaves a gated session gated). Found by the review on this PR.
Adversarial review of
|
The fold added in the previous commit hardened `default` and `deny` but took the document's `allow`, `proxy`, `proxy_fake_ip_cidrs`, and `audit` wholesale. `decide` checks `allow` *before* falling back to `default`, so under a deny-by-default authority — the canonical managed / Fleet stance — a document listing `allow = ["api.github.com"]` still produced `Allow`. The fold was escapable in the one direction it exists to protect, and the review that found it also caught that the case's own test asserted the leak: it checked `folded.default` stayed `Deny` and that the document's host was in `allow`, never what `decide` returned. The fold is now asymmetric by the authority's own fallback: - `deny`: union. Either layer saying no is a no. - `default`: the stricter of the two, as before. - Authority `default = Deny`: the lower layer contributes nothing that can allow. `allow`, `proxy`, and the fake-IP CIDRs stay the authority's own — no host lifted, no SSRF exception granted. - Authority `default = Prompt`: the lower layer may name hosts, which is what `/network allow <host>` writes. Those **merge** with the authority's own instead of replacing them, so a refresh cannot erase an administrator's allowances either. - `audit`: either layer asking for the log keeps it. A document cannot silence audit under an authority. `folding_a_document_never_widens_an_authority` now asserts `decide()` verdicts for both fallbacks rather than field shape: the document's host is `Deny` under a denial and `Allow` under a prompt, the authority's own allowance survives, an unlisted host keeps the authority's fallback, and no fake-IP exception or audit silencing crosses the fold. `cargo test -p codewhale-tui network_policy` -> 40 passed (lib) + 28 passed (integration); `cargo test -p codewhale-tui tool_context` -> 24 passed.
Review: PR #8 third commit
|
| auth def | doc def | folded default | doc-only-host verdict | > authority-alone? |
|---|---|---|---|---|
| Deny | Deny | Deny | Deny (frozen) | no |
| Deny | Prompt | Deny | Deny (frozen) | no |
| Deny | Allow | Deny | Deny (frozen) | no |
| Prompt | Deny | Deny | Allow (union) | yes → doc chose deny-default yet named allow |
| Prompt | Prompt | Prompt | Allow (union) | yes → intended /network allow |
| Prompt | Allow | Prompt | Allow (union) | yes → intended /network allow |
| Allow | Deny | Deny | Allow (union) | no |
| Allow | Prompt | Prompt | Allow (union) | no |
| Allow | Allow | Allow | Allow (union) | no |
The Prompt rows are the intended /network allow behavior — an authority
that is not deny-by-default tolerates new hostnames. The two rows where the
folded default becomes Deny but the document's allow survives both have the
document itself setting its own default = Deny. The user named those hosts;
they are not a widening. Starting the authority's own default — the only one
whose Deny is a boundary the user may not cross — is the right key for the
"fleet denial" authority, which always resolves to Deny via
exec_network_policy (lib.rs:14743). That case is fully sealed.
2. /network allow reports success, writes an inert entry, and tells you nothing
** — product gap, confirmed, pre-existing (not introduced here).** A Fleet
denial (outer_network_access == Some(false)) is frozen: allow stays the
authority's own. But /network allow <host> is a document-only
writer (crates/tui/src/commands/groups/utility/network.rs:162):
"Network host {action}: {host}\nSaved to {path}. Retry the command now."
Every code path — the doc now has the host under [network].allow, a re-read
folds it, and folded.allow does not include it because it is frozen. It prints
"Network host allowed: {host}" and "Saved", which reads as success even though
the allow never takes effect. /network list labels that allow entry as live.
Worse, the NetworkDenied hint (contract.rs:2850, plugins/mod.rs:927)
tells the user to "Add it to your allow list with /network allow {host}, then
retry" — precisely the inert action, sent as if it is the fix. The engine
frozen correctly; the command-side UI is what is misled. Worth a follow-up so
that /network allow/list can surface "a Fleet denial keeps the last word; this
entry is stored but has no effect", but that is a separate product fix, not a
defect in this commit.
3. The fold's audit = true interacts correctly with with_policy_refreshed; the one dead-audit case is pre-existing
** — confirmed consistent; one adjacent pre-existing gap.** The refreshed
decider (network_policy.rs:636) re-derives trusted_fakeip_cidrs from the
folded policy and rebuilds the auditor against policy.audit_enabled(), so a
fold that clears proxy_fake_ip_cidrs (deny-authority) leaves no trusted CIDRs
(with_policy_refreshed re-parses the folded list, so a cleared list clears
trust), and a fold that raises audit to true yields a new enabled
auditor (None & policy.audit_enabled() => default_path(true)). No stale-CIDR
or dead-audit-under-audit-true path is opened by this commit.
Pre-existing and separable: the Fleet-denial authority is built with
NetworkPolicyDecider::new(policy, None) (lib.rs:14741), so it has
policy.audit == true (Default) but no auditor, and audit_record is a
no-op when self.auditor is None (network_policy.rs:735) until a document
re-read triggers a refresh that materializes the auditor. A long-gated session
never writes an audit line despite audit_enabled() == true. Independent of
this commit; naming it for completeness.
4. The rewritten test pins the verdicts — and the two field asserts are genuine (the Default-value objection fails here)
folding_a_document_never_widens_an_authority (network_policy.rs:1036) now
walks decide() (the "F1-as-test-asserted-the-leak" defect is gone —
the deny-authority verdicts, 1060/1065/1070 pin DenyDenyDeny; the
prompt-authority ones, 1097/1102/1107/1112 pin Allow/Allow/Prompt/Deny).
proxy_fake_ip_cidrsasserts (1074–1076): these are field asserts
becausedecidetakes only hosts; a CIDR cannot go through it. But they do not
test aDefault— the deny-branch would have discarded the doc's CIDR,
otherwise the doc's198.18.0.0/15would be present and the assert would
fail. It genuinely pins the "a doc cannot hand itself an SSRF exception"
freeze.folded.audit(1078–1079): asserts a doc withaudit: falsecannot
silence an authority withaudit: true. That is a real pin on the
self.audit || lower.auditwider direction it names. The complementary
direction (authority off, doc on → union keeps it on) is unpinned but is
more-logging-not-more-access, harmless.
Unpinned corner, worth a guard test: the authority-Allow default key
(row 7–9 above) is a valid live state (a managed [network] overlay resolving
default = "allow", via into_runtime at config.rs:3057 and
network_layer_is_managed at config.rs:11014). Its fold uses the union
branch; combined with a doc default = Deny it yields a deny-by-default
folded policy that still honors the document's allow. That is not more
permissive than the authority-alone (Allow allows everything), so it is not
F1-class, but a guard test asserting that case's verdict would pin the intent
explicitly instead of leaving it by silhouette.
Everything else — nothing found
stricter_decision (statically confirmed): Deny < Prompt < Allow, returns the
lower-rank operand in both argument orders, ties (equal rank) return the left
which is identical to the right. Order-dependent. It is used only for default,
whose result is always the stronger of the two — the document can make the
fallback more restrictive, never loose. That is the "may narrow, never widen a
denial", read directly.
I did not find any residual in: the self.audit || lower.audit override
(either layer may keep audit); the freeze's interaction with decide's
empty-host special case; the session cache (approve_session/deny_session
riding the refresh) — none of these can resurrect a frozen allow.
No Rust toolchain on this box — careful static pass with the quoted
evidence above, no build or run.
Attribution
🤖 Generated by SpikeBot 003(ClaudeCode-JP)
The review noted the authority-Allow fallback branch was correct but unpinned: a managed overlay may legitimately resolve `default = "allow"`, and its fold takes the union branch. The test asserts the document's own `default = deny` narrows it and a named host stays allowed -- nothing there is more permissive than the authority's own policy, because that policy already allowed everything. `cargo test -p codewhale-tui network_policy` -> 41 passed (lib) + 29 passed (integration).
1 participant
Summary
/network allow <host>writes the host intoconfig.tomland tells the operatorSaved to ... Retry the command now.— but the retry was still refused withrequires network approval. The engine captures itsNetworkPolicyDecideronce,when the session is spawned, and the slash command only edits the document, so
nothing read the file again for the rest of the session. The host stayed blocked
until CodeWhale was restarted, which is exactly what the message promises the
operator they do not have to do.
This makes the tool context (and the MCP pool) re-read the
[network]table fromthe document the session was launched with, so the retry sees the host the file
already allows.
Reproduction
api.github.comin[network] allow(or run/network allow api.github.com).ghcommand, e.g.gh repo view owner/repo.Before:
Read-only GitHub CLI access to 'api.github.com' requires network approval; allow that host in the parent session or network policy before dispatching the scout.The file on disk was correct the whole time, and a plain
Invoke-WebRequesttohttps://api.github.com/rate_limitreturned 200 from the same machine — so thegate, not the network, was the blocker.
Changes
NetworkPolicyDecider::with_policy_refreshed: rebuilds a decider against afreshly parsed policy while keeping the session cache, so an approval already
granted through the approval prompt survives the refresh. The auditor keeps its
log path and follows the new policy's
auditswitch.config::network_policy_from_document: re-reads only the document's own[network]table. Deliberately narrower thanConfig::load— the tool-contextbuild path must not re-apply environment, managed, and credential layers. A
missing/unreadable/unparseable document, or one with no
[network]table,returns
Noneand the caller keeps the policy it already holds, which is theconservative direction for an allow/deny gate.
Engine::current_network_decider(new) + two call sites incrates/tui/src/core/engine.rs: the per-turn tool context, and the MCP pool atconstruction time.
/network deny <host>writes the same[network]table
/network allowdoes, so a session that started without one adopts thetable the moment it appears. Otherwise tightening a policy mid-session would
need the very restart this change removes. A session that started gated keeps
the policy it holds if the table later disappears, so removing
[network]cannot leave a live session ungated by accident.
After review
The adversarial review found that the first version left the deny direction
cold: it short-circuited on a session whose spawn-time policy was
None, so amid-session
[network] denywas ignored exactly when a policy was beingtightened. The independent review found something more serious in the same
code — a resolved authority could be widened:
[network]table(
apply_managed_overrides).network_access = falsenever consults that table at all;exec_network_policy(crates/tui/src/lib.rs) synthesizes adefault = denydecider, and its own comment says user configuration "maynever widen an explicit network denial".
Replacing the resolved policy with the raw document therefore turned an
administrator's denial into prompt/allow on the first tool-context build —
every turn. The second commit closed both by folding the re-read onto the
resolved policy instead of replacing it.
A third commit closes what the second one still let through. The fold
hardened
defaultanddenybut took the document'sallow,proxy,proxy_fake_ip_cidrs, andauditwholesale — anddecidechecksallowbefore falling back to
default, so under a deny-by-default authority adocument listing
allow = ["api.github.com"]still producedAllow. The foldwas escapable in the one direction it exists to protect. The fold is now
asymmetric by the authority's own fallback:
deny— union;default— the stricter of the two.default = Deny: the lower layer contributes nothing that canallow.
allow,proxy, and the fake-IP CIDRs stay the authority's own, sono host is lifted and no SSRF exception is granted.
default = Prompt: the lower layer may name hosts, which is what/network allow <host>writes, and those merge with the authority's ownrather than replacing them — a refresh cannot erase an administrator's
allowances either.
audit: either layer asking for the log keeps it.That third commit's own test had asserted the leak (
folded.defaultstayedDenyand the document's host was inallow— never whatdecidereturned).It now asserts verdicts for both fallbacks.
Follow-up this leaves open (pre-existing, not introduced here)
Under a Fleet denial, the fold correctly freezes
allow, and that makes/network allow <host>write an entry that has no effect — while the commandstill prints
Network host allowed: <host>andSaved to ..., and/network listshows the entry as live. TheNetworkDeniedhint even tells theoperator to run exactly that command. The gate is right; the command-side UI is
misleading. Surfacing it needs
/networkto know whether an authority owns thepolicy, and it is a
CommandHandler::Purewith no engine handle — so it is aseparate fix, not part of this one.
Scope limit worth naming
The per-turn tool context — the path every shell, web, finance, speech, and skill
call goes through, including the reported read-only
ghrefusal — re-reads onevery turn. The MCP pool does not:
ensure_mcp_poolreturns early once a poolexists, so its decider is the one captured when the pool was built. A
/network allowissued after the pool exists therefore does not reach MCP transports untilthe pool is rebuilt. Fixing that means mutating a live pool's policy, which this
change deliberately does not do.
An adopted table has no session cache to inherit, because the session had no
decider and therefore nothing was ever approved under one.
Why a re-read rather than wiring the slash command to the engine
/networkis registered asCommandHandler::Pure— it has noApp, no enginehandle, and no channel to a live decider by construction. The neighbouring
WorkspaceTrustlist solves the same problem the same way: it is re-read from diskon every tool-context build so
/trust addlands mid-session. This follows thatprecedent, and as a side benefit a hand edit of
[network]inconfig.tomlalsotakes effect on the next turn instead of the next launch.
Testing
cargo fmt --all -- --check— clean.cargo test -p codewhale-tui network_policy— 41 passed; 0 failed in thelib target (14,769 filtered out) and 29 passed; 0 failed in the
integration target, including five new unit tests:
refreshed_policy_adopts_new_table_without_retracting_session_approvalsrefreshed_policy_honours_a_new_deny_listrefreshed_policy_keeps_the_authority_markingfolding_a_document_never_widens_an_authority— asserts the folded policy'sverdicts for both fallbacks: the document's host is
Denyunder adenial and
Allowunder a prompt, the authority's own allowance survives,an unlisted host keeps the authority's fallback, and no fake-IP exception or
audit silencing crosses the fold.
folding_an_allow_by_default_authority_only_narrows— thedefault = "allow"branch (a managed overlay may resolve it): the documentcan only narrow the fallback, and a host it names stays allowed.
cargo test -p codewhale-tui tool_context— 24 passed; 0 failed.New integration regression tests in
crates/tui/src/core/engine/tests/test_cases_12.rs:tool_context_network_policy_follows_the_config_document_mid_session— buildsa real
Enginewhose spawn-time policy does not allowapi.github.com,points
loaded_config_pathat a document that does, and asserts the toolcontext that comes back evaluates
api.github.comasAllowwhile anunlisted host still prompts.
tool_context_adopts_a_policy_written_mid_session_and_never_ungates_one— asession that started with no
[network]table adopts a document'sdeny = ["evil.example.com"], and a session that started gated keeps itspolicy after the table is removed.
On the "prove it fails without the fix" rule: the old code path clones
self.config.network_policy— the spawn-time snapshot, which in the firstfixture carries
default = "prompt"and no allow entry — so the sameassertion would read
Promptand fail. That chain is pinned by existing tests(
unknown_host_returns_default), but I did not re-run the suite against areverted build to demonstrate it, and am not claiming that receipt.
Known limitation
[profiles.<name>.network]is not consulted./networkdoes not write that layereither, so a live session and the command agree on the base table. Documented in
the new function's doc comment rather than left implicit.
Base and scope
This PR targets this fork (
SparkofSpike/CodeWhale), basemain— which isfast-forwarded to upstream
main(9524531de) so the diff is exactly thisbranch. It is not proposed upstream here.
The second and third commits answer both reviews: the adversarial review's
deny-direction and fold-escape findings, and the independent review's
authority-widening finding. All of them, and the fixes, are in the PR thread.
Type of Change
Checklist
the test receipt above plus the reproduction steps
Related Issues
No-Issue: reproduced from an operator session on 0.10.1; no upstream issue filed
for it yet.
Attribution
🤖 Generated by SpikeBot 000(CodeWhale-LOCAL)