From 13c51fdb5c8fd8c40e6ae69afc6f94cf6942e832 Mon Sep 17 00:00:00 2001 From: Tom Maiaroto Date: Tue, 20 Oct 2026 20:23:17 -0700 Subject: [PATCH 1/7] fix: close egress firewall and provider-wire redaction gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IPv6 literals no longer bypass the web_fetch raw-IP block, and tool-call inputs and text results are now masked at the provider wire like user text. go.mod bumped to 1.27.3 for the patched trust store (eMudhra CA revocations). ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- docs/security/NETWORK_EGRESS_HARDENING.md | 35 ++++++-- .../SECURITY_VALIDATION_TEST_SUITE.md | 4 +- go.mod | 2 +- pkg/agent/outgoing_redact.go | 41 +++++++-- pkg/agent/outgoing_redact_test.go | 86 +++++++++++++++++++ pkg/agent/tools/web_fetch.go | 20 +++-- pkg/agent/tools/web_fetch_test.go | 69 +++++++++++++++ .../security/NETWORK_EGRESS_HARDENING.md | 35 ++++++-- 8 files changed, 257 insertions(+), 35 deletions(-) diff --git a/docs/security/NETWORK_EGRESS_HARDENING.md b/docs/security/NETWORK_EGRESS_HARDENING.md index 787b8f9..f5488d0 100644 --- a/docs/security/NETWORK_EGRESS_HARDENING.md +++ b/docs/security/NETWORK_EGRESS_HARDENING.md @@ -172,19 +172,38 @@ guarantees. ## Security Properties - **Fail-closed**: no network requests succeed until the agent confirms the domain is trusted -- **IP-block policy**: raw IP addresses (e.g., `10.x.x.x`, `192.168.x.x`, public IPs) are rejected — FQDNs required +- **IP-block policy**: raw IP addresses are rejected for both IPv4 and IPv6 literal forms (e.g., `10.x.x.x`, `192.168.x.x`, `[::1]`, `[fd00::…]`) — FQDNs required - **Opt-in overrides**: `AllowRawIPs` + `IPAllowList` (CIDR) allow raw IPs for local/network development - **Layered trust**: workspace + global allow lists + session allow list + user prompt - **OTel observability**: every allow/deny/event is captured as an OTel span with structured attributes - **Cacheable**: user approvals persist across requests (in-memory cache) but are session-local - **Shared transport**: both `web_fetch` and `web_search` share the same `*http.Client` → same `securityTransport` -- **Fail-closed**: no network requests succeed until the agent confirms the domain is trusted -- **IP-block policy**: raw IP addresses (e.g., `10.x.x.x`, `192.168.x.x`, public IPs) are rejected — FQDNs required -- **Layered trust**: workspace + global allow lists + session allow list + user prompt -- **OTel observability**: every allow/deny/event is captured as an OTel span with structured attributes -- **Cacheable**: user approvals persist across requests (in-memory cache) but are session-local -- **Shared transport**: both `web_fetch` and `web_search` share the same `*http.Client` → same `securityTransport` +## TLS Trust and CA Compromise + +The IP-block policy and trust hierarchy govern *where* the agent may connect; +certificate validation governs *whether the peer is who it claims to be*. The +two layers are independent, and an AI agent depends on the second more than +ordinary software does: the agent carries provider API keys in every request, +fetches attacker-chosen URLs by design, and can download and execute code from +hosts it has never seen. A compromised certificate authority — one whose +intermediate keys leak and can sign certs for *any* domain — silently defeats +every hostname-level defense at once (allow lists, IP blocks, FQDN +requirements). + +Phosphor performs no custom certificate handling. TLS verification is delegated +to the runtime trust store: + +- **Windows / macOS**: the platform certificate store, so OS security updates + and browser-grade revocation (CRLSets, OneCRL) cover CA compromise. +- **Linux**: Phosphor builds with `CGO_ENABLED=0`, so Go uses its compiled-in + copy of the Mozilla root program. Keeping the Go toolchain patched — + `go.mod` pins the minimum version — is therefore part of the security + posture, not just a build detail. + +Concretely: when an intermediate CA in a trusted root program is compromised, +the remedy is a toolchain/OS update, and users should update promptly the same +way they would for a browser. ## Files Modified @@ -195,7 +214,7 @@ guarantees. ## Notes -- `go.mod` requires Go 1.26.4+ (running 1.25.0 locally — LSP errors may appear for tooling checks) +- `go.mod` requires Go 1.27.3+ (patched trust store; see TLS Trust and CA Compromise above) - `fmt`, `net`, `strings`, `html/template`, `encoding/json`, `time`, `embed` imports are all used in body - `maybeDelaySearch` removed from `web_search.go` (already present in `search.go`) - `randomDelay` added for DDG request-budget threshold diff --git a/docs/security/SECURITY_VALIDATION_TEST_SUITE.md b/docs/security/SECURITY_VALIDATION_TEST_SUITE.md index 591f4a1..7fbc931 100644 --- a/docs/security/SECURITY_VALIDATION_TEST_SUITE.md +++ b/docs/security/SECURITY_VALIDATION_TEST_SUITE.md @@ -120,7 +120,7 @@ Validates that IsInside and SmartJoin handle complex path resolutions robustly. | `TestPathObfuscation_SymlinkTraversal` | Symlinks pointing outside workspace resolved by EvalSymlinks and blocked | | `TestPathObfuscation_AbsolutePathVariants` | Absolute paths on all platforms blocked: Windows (`C:\Windows\...`), Linux (`/etc/passwd`), macOS (`/Users/...`) | -### `internal/agent/tools/web_fetch_test.go` +### `pkg/agent/tools/web_fetch_test.go` Validates network egress hardening: the `securityTransport` interceptor, IP-block policy, CIDR allow list, localhost exceptions, and TUI permission prompt. @@ -129,6 +129,8 @@ Validates network egress hardening: the `securityTransport` interceptor, IP-bloc | `TestWebFetchHardeningIPBlock` | Raw IP addresses (e.g. `192.168.1.1`) rejected — FQDN required | | `TestWebFetchHardeningCIDRAllow` | CIDR allow list (`192.168.1.0/24`) respected — IP falls in range allowed | | `TestWebFetchHardeningLocalhostAlwaysAllowed` | `127.0.0.1` / `::1` always allowed — no config needed | +| `TestWebFetchHardeningIPv6LiteralBlocked` | Bracketed IPv6 literals (`[::1]:6379`, `[fd00::…]`) rejected as raw IPs — never reach the trust hierarchy | +| `TestWebFetchHardeningIPv6LocalhostAllowed` | `[::1]` treated as localhost — passes the gate without a prompt | | `TestWebFetchHardeningUserPromptVerified` | Unknown host triggers TUI prompt — user granted access | | `TestWebFetchHardeningUserPromptDenyVerified` | Unknown host triggers TUI prompt — user denied access → request blocked | diff --git a/go.mod b/go.mod index 1fb8120..03f2b55 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/hackafterdark/phosphor -go 1.27.0 +go 1.27.3 require ( charm.land/bubbles/v2 v2.2.1 diff --git a/pkg/agent/outgoing_redact.go b/pkg/agent/outgoing_redact.go index 10eaaea..1a5930b 100644 --- a/pkg/agent/outgoing_redact.go +++ b/pkg/agent/outgoing_redact.go @@ -149,17 +149,40 @@ func (a *sessionAgent) redactOutgoingMessages(msgs []fantasy.Message) []fantasy. parts := make([]fantasy.MessagePart, len(msg.Content)) changed := false for j, part := range msg.Content { - tp, ok := part.(fantasy.TextPart) - if !ok { + switch p := part.(type) { + case fantasy.TextPart: + next := a.redactOutgoingText(p.Text) + if next != p.Text { + p.Text = next + changed = true + } + parts[j] = p + case fantasy.ToolCallPart: + // Tool-call inputs (bash command lines, MCP arguments, edit + // bodies) are replayed verbatim on every subsequent request; + // a credential typed into an argument would otherwise ride + // the wire in plaintext forever. + next := a.redactOutgoingText(p.Input) + if next != p.Input { + p.Input = next + changed = true + } + parts[j] = p + case fantasy.ToolResultPart: + // Text results are normally masked per-tool, but anything + // that reaches a result outside the redacted surfaces must + // still meet the last-resort mask at the wire. + if txt, ok := fantasy.AsToolResultOutputType[fantasy.ToolResultOutputContentText](p.Output); ok { + next := a.redactOutgoingText(txt.Text) + if next != txt.Text { + p.Output = fantasy.ToolResultOutputContentText{Text: next} + changed = true + } + } + parts[j] = p + default: parts[j] = part - continue - } - next := a.redactOutgoingText(tp.Text) - if next != tp.Text { - tp.Text = next - changed = true } - parts[j] = tp } if changed { out[i].Content = parts diff --git a/pkg/agent/outgoing_redact_test.go b/pkg/agent/outgoing_redact_test.go index b686606..3fcc47b 100644 --- a/pkg/agent/outgoing_redact_test.go +++ b/pkg/agent/outgoing_redact_test.go @@ -335,3 +335,89 @@ func TestSessionAgent_OutgoingRedaction_ForceOnlyMasksThroughCallSite(t *testing require.NotContains(t, textOf(out[0]), secret) require.Contains(t, textOf(out[0]), " Date: Tue, 20 Oct 2026 20:56:07 -0700 Subject: [PATCH 2/7] fix: route summarize and title calls through wire redaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The summarizer and title-generator calls shipped the full history and raw user prompt to the small model with no defang or last-resort secret/PII mask. Both now run through the same wire defenses as the main agent loop, pinned by new regression tests. ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- pkg/agent/agent.go | 12 ++- pkg/agent/wire_mask_small_model_test.go | 98 +++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 pkg/agent/wire_mask_small_model_test.go diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 4b5d176..d25fdfd 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -2196,6 +2196,14 @@ func (a *sessionAgent) Summarize(ctx context.Context, sessionID string, opts fan if systemPromptPrefix != "" { prepared.Messages = append([]fantasy.Message{fantasy.NewSystemMessage(systemPromptPrefix)}, prepared.Messages...) } + // Route the summarizer request through the same wire defenses as + // the main agent loop: the full history (including raw stored + // reasoning) rides this call, so it must meet the defang and the + // last-resort secret/PII mask too. + prepared.Messages = defangOutgoingMessages(prepared.Messages) + if a.outgoingRedactionEnabled() { + prepared.Messages = a.redactOutgoingMessages(prepared.Messages) + } return callContext, prepared, nil }, OnReasoningDelta: func(id string, text string) error { @@ -2914,7 +2922,9 @@ func (a *sessionAgent) GenerateTitle(ctx context.Context, sessionID string, user } streamCall := fantasy.AgentStreamCall{ - Prompt: userPrompt, + // The raw user prompt rides this call verbatim; give it the same + // defang and last-resort mask the main loop applies to history. + Prompt: a.redactOutgoingText(tools.DefangSpecialTokens(userPrompt)), } var resp *fantasy.AgentResult diff --git a/pkg/agent/wire_mask_small_model_test.go b/pkg/agent/wire_mask_small_model_test.go new file mode 100644 index 0000000..82cf298 --- /dev/null +++ b/pkg/agent/wire_mask_small_model_test.go @@ -0,0 +1,98 @@ +package agent + +import ( + "context" + "strings" + "sync" + "testing" + + "charm.land/fantasy" + "github.com/hackafterdark/phosphor/pkg/message" + "github.com/stretchr/testify/require" +) + +// recordingStreamModel captures every fantasy.Call handed to it so tests can +// assert on the exact payload about to go over the wire. +type recordingStreamModel struct { + finishStreamModel + mu sync.Mutex + calls []fantasy.Call +} + +func (m *recordingStreamModel) Stream(ctx context.Context, call fantasy.Call) (fantasy.StreamResponse, error) { + m.mu.Lock() + m.calls = append(m.calls, call) + m.mu.Unlock() + return m.finishStreamModel.Stream(ctx, call) +} + +func (m *recordingStreamModel) recordedText() string { + m.mu.Lock() + defer m.mu.Unlock() + var sb strings.Builder + for _, call := range m.calls { + for _, msg := range call.Prompt { + for _, part := range msg.Content { + if tp, ok := part.(fantasy.TextPart); ok { + sb.WriteString(tp.Text) + sb.WriteString("\n") + } + } + } + } + return sb.String() +} + +const wireMaskSecret = "AKIAIOSVPK253PIP5TGP" + +func TestSummarize_AppliesWireRedactionToHistory(t *testing.T) { + t.Parallel() + + env := testEnv(t) + rec := &recordingStreamModel{finishStreamModel: finishStreamModel{text: "summary"}} + agent := testSessionAgent(env, rec, rec, "system").(*sessionAgent) + agent.wireSecretsForced = true + + session, err := env.sessions.Create(t.Context(), "New Session") + require.NoError(t, err) + + _, err = env.messages.Create(t.Context(), session.ID, message.CreateMessageParams{ + Role: message.User, + Parts: []message.ContentPart{ + message.TextContent{Text: "AWS_ACCESS_KEY_ID := \"" + wireMaskSecret + "\""}, + }, + }) + require.NoError(t, err) + + err = agent.Summarize(t.Context(), session.ID, nil) + require.NoError(t, err) + + got := rec.recordedText() + require.NotEmpty(t, got, "summarizer must have been called") + require.NotContains(t, got, wireMaskSecret, + "summarizer request must not carry the raw secret") + require.Contains(t, got, " Date: Tue, 20 Oct 2026 21:10:16 -0700 Subject: [PATCH 3/7] fix: validate resolved dial addresses against the IP-block policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The egress firewall validated hostnames as strings, so a DNS answer that rebound an approved hostname into a private, link-local, or cloud-metadata range was never checked (DNS rebinding). web_fetch now gates the actual socket address at dial time with the same policy as raw IPs — loopback allowed, allow_raw_ips/ip_allow_list exempt, denied ranges refused. The web_fetch tool config is wired through, so the documented allow-list options take effect. ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- docs/security/NETWORK_EGRESS_HARDENING.md | 1 + .../SECURITY_VALIDATION_TEST_SUITE.md | 3 + pkg/agent/agentic_fetch_tool.go | 2 +- pkg/agent/tools/url_secret_guard_test.go | 2 +- pkg/agent/tools/web_fetch.go | 101 +++++++++++++++++- pkg/agent/tools/web_fetch_test.go | 97 +++++++++++++++++ .../security/NETWORK_EGRESS_HARDENING.md | 1 + 7 files changed, 203 insertions(+), 4 deletions(-) diff --git a/docs/security/NETWORK_EGRESS_HARDENING.md b/docs/security/NETWORK_EGRESS_HARDENING.md index f5488d0..e787620 100644 --- a/docs/security/NETWORK_EGRESS_HARDENING.md +++ b/docs/security/NETWORK_EGRESS_HARDENING.md @@ -173,6 +173,7 @@ guarantees. - **Fail-closed**: no network requests succeed until the agent confirms the domain is trusted - **IP-block policy**: raw IP addresses are rejected for both IPv4 and IPv6 literal forms (e.g., `10.x.x.x`, `192.168.x.x`, `[::1]`, `[fd00::…]`) — FQDNs required +- **Resolved-address validation**: the same IP policy is enforced at dial time on the *resolved* socket address, so an approved hostname whose DNS answer rebinds into a private, link-local, or cloud-metadata range is refused at the socket (DNS-rebinding defense). Loopback stays allowed; `IPAllowList`/`AllowRawIPs` exempt like raw IPs; when an environment proxy is configured the operator's proxy is the egress decision and the strict path remains the egress broker tier - **Opt-in overrides**: `AllowRawIPs` + `IPAllowList` (CIDR) allow raw IPs for local/network development - **Layered trust**: workspace + global allow lists + session allow list + user prompt - **OTel observability**: every allow/deny/event is captured as an OTel span with structured attributes diff --git a/docs/security/SECURITY_VALIDATION_TEST_SUITE.md b/docs/security/SECURITY_VALIDATION_TEST_SUITE.md index 7fbc931..7ad0cc0 100644 --- a/docs/security/SECURITY_VALIDATION_TEST_SUITE.md +++ b/docs/security/SECURITY_VALIDATION_TEST_SUITE.md @@ -131,6 +131,9 @@ Validates network egress hardening: the `securityTransport` interceptor, IP-bloc | `TestWebFetchHardeningLocalhostAlwaysAllowed` | `127.0.0.1` / `::1` always allowed — no config needed | | `TestWebFetchHardeningIPv6LiteralBlocked` | Bracketed IPv6 literals (`[::1]:6379`, `[fd00::…]`) rejected as raw IPs — never reach the trust hierarchy | | `TestWebFetchHardeningIPv6LocalhostAllowed` | `[::1]` treated as localhost — passes the gate without a prompt | +| `TestWebFetchDialGateResolvedAddress` | DNS-rebinding defense: hostnames resolving to private/link-local/metadata ranges refused at the socket; loopback and public addresses pass | +| `TestWebFetchDialGateHonorsConfigOverrides` | `allow_raw_ips` and `ip_allow_list` CIDRs exempt the dial gate, matching the raw-IP policy | +| `TestWebFetchDialGateLocalhostHostnameAllowed` | Local dev flows survive the default gate — `127.0.0.1` httptest server reachable through the guarded dialer | | `TestWebFetchHardeningUserPromptVerified` | Unknown host triggers TUI prompt — user granted access | | `TestWebFetchHardeningUserPromptDenyVerified` | Unknown host triggers TUI prompt — user denied access → request blocked | diff --git a/pkg/agent/agentic_fetch_tool.go b/pkg/agent/agentic_fetch_tool.go index 5c075dc..e2b0a30 100644 --- a/pkg/agent/agentic_fetch_tool.go +++ b/pkg/agent/agentic_fetch_tool.go @@ -172,7 +172,7 @@ func (c *coordinator) agenticFetchTool(_ context.Context, client *http.Client) ( return fantasy.ToolResponse{}, errors.New("small model provider not configured") } - webFetchTool := tools.NewWebFetchTool(tmpDir, client, c.tuiAllowPrompt) + webFetchTool := tools.NewWebFetchTool(tmpDir, client, c.cfg.Config(), c.tuiAllowPrompt) webSearchTool := tools.NewWebSearchTool(client) fetchTools := []fantasy.AgentTool{ webFetchTool, diff --git a/pkg/agent/tools/url_secret_guard_test.go b/pkg/agent/tools/url_secret_guard_test.go index e10db63..b62b4db 100644 --- a/pkg/agent/tools/url_secret_guard_test.go +++ b/pkg/agent/tools/url_secret_guard_test.go @@ -67,7 +67,7 @@ func TestFetchTool_AllowsCleanQuery(t *testing.T) { func TestWebFetchTool_RefusesRegisteredSecretInQuery(t *testing.T) { allowList = make(map[string]bool) - tool := NewWebFetchTool(t.TempDir(), nil, allowAllHosts) + tool := NewWebFetchTool(t.TempDir(), nil, nil, allowAllHosts) input, err := json.Marshal(WebFetchParams{URL: "https://example.com/dl?sig=" + guardToken}) require.NoError(t, err) diff --git a/pkg/agent/tools/web_fetch.go b/pkg/agent/tools/web_fetch.go index ca67976..d620165 100644 --- a/pkg/agent/tools/web_fetch.go +++ b/pkg/agent/tools/web_fetch.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "time" _ "embed" @@ -319,11 +320,107 @@ func matchCIDR(cidr, host string) bool { return network.Contains(ip) } +// webFetchDeniedCIDRs are the destination ranges refused at dial time when a +// hostname resolves into them (DNS rebinding defense). Loopback is exempt: +// web_fetch's raw-IP policy always allows localhost, and local dev servers are +// legitimate hostnames. +var webFetchDeniedCIDRs = []string{ + "0.0.0.0/8", // "this" network + "10.0.0.0/8", // private + "100.64.0.0/10", // CGNAT + "169.254.0.0/16", // link-local (incl. cloud metadata 169.254.169.254) + "172.16.0.0/12", // private + "192.168.0.0/16", // private + "::/128", // IPv6 unspecified + "FC00::/7", // IPv6 unique-local + "FE80::/10", // IPv6 link-local +} + +// envProxyConfigured reports whether the environment names an HTTP(S) proxy. +// When a proxy is set, the address web_fetch dials is the proxy's, not the +// target's, and private proxy infrastructure is the operator's own network +// decision; the egress broker tier remains the strict path for that case. +func envProxyConfigured() bool { + for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY"} { + if v := os.Getenv(k); v != "" { + return true + } + if v := os.Getenv(strings.ToLower(k)); v != "" { + return true + } + } + return false +} + +// dialAddressAllowed validates the *resolved* socket address of a web_fetch +// dial. The trust hierarchy judges hostnames as strings; this gate judges the +// actual connection target, so a DNS answer that rebinds an approved hostname +// into a private, link-local, or metadata range is refused at the socket +// instead of silently connecting. The resolved address is held to the same +// policy as a raw-IP literal: loopback always allowed, configured allow-list +// (and `allow_raw_ips`) exempt, denied ranges refused, public addresses pass. +func dialAddressAllowed(address string, cfg *config.Config) bool { + host, _, err := net.SplitHostPort(address) + if err != nil { + host = address + } + ip := net.ParseIP(strings.Trim(host, "[]")) + if ip == nil { + return true + } + if v4 := ip.To4(); v4 != nil { + ip = v4 + } + if ip.IsLoopback() { + return true + } + if cfg != nil { + if cfg.Tools.WebFetch.AllowRawIPs { + return true + } + for _, cidr := range cfg.Tools.WebFetch.IPAllowList { + if matchCIDR(cidr, ip.String()) { + return true + } + } + } + for _, cidr := range webFetchDeniedCIDRs { + if matchCIDR(cidr, ip.String()) { + return false + } + } + return true +} + +// guardedDialer returns a dialer whose ControlContext validates the resolved +// socket address of every web_fetch connection. ControlContext runs after the +// kernel has dialed but before any data flows, so the address judged is the +// address actually used — there is no re-resolution race to exploit. +func guardedDialer(cfg *config.Config) *net.Dialer { + return &net.Dialer{ + ControlContext: func(_ context.Context, _, address string, _ syscall.RawConn) error { + if envProxyConfigured() { + return nil + } + if !dialAddressAllowed(address, cfg) { + host, _, _ := net.SplitHostPort(address) + return fmt.Errorf( + "web_fetch request to %s blocked (resolved address is in a denied range; DNS rebinding defense)", + host, + ) + } + return nil + }, + KeepAlive: egress.DefaultKeepAlive, + } +} + // NewWebFetchTool constructs a web fetch tool that uses the security transport // and invokes the TUI permission prompt when a host is not yet approved. -func NewWebFetchTool(workingDir string, client *http.Client, allowFn func(ctx context.Context, host string) (bool, error)) fantasy.AgentTool { +func NewWebFetchTool(workingDir string, client *http.Client, cfg *config.Config, allowFn func(ctx context.Context, host string) (bool, error)) fantasy.AgentTool { baseTransport := egress.NewHTTPTransport() - secure := newSecurityTransport(egress.Transport(baseTransport), allowFn, nil) + baseTransport.DialContext = guardedDialer(cfg).DialContext + secure := newSecurityTransport(egress.Transport(baseTransport), allowFn, cfg) secureClient := &http.Client{ Timeout: 30 * time.Second, Transport: secure, diff --git a/pkg/agent/tools/web_fetch_test.go b/pkg/agent/tools/web_fetch_test.go index c155301..3292dbd 100644 --- a/pkg/agent/tools/web_fetch_test.go +++ b/pkg/agent/tools/web_fetch_test.go @@ -9,6 +9,8 @@ import ( "time" "github.com/hackafterdark/phosphor/pkg/config" + "github.com/hackafterdark/phosphor/pkg/egress" + "github.com/stretchr/testify/require" ) // TestWebFetchHardeningIPBlock demonstrates the IP-block policy rejects raw IPs. @@ -246,3 +248,98 @@ func TestWebFetchHardeningIPv6LocalhostAllowed(t *testing.T) { resp.Body.Close() } } + +// TestWebFetchDialGateResolvedAddress is the DNS-rebinding regression table: +// an approved hostname whose DNS answer lands in a denied range must be +// refused at the socket, while loopback, config-allowlisted, and public +// addresses pass. +func TestWebFetchDialGateResolvedAddress(t *testing.T) { + t.Parallel() + + cases := []struct { + address string + allowed bool + }{ + {"127.0.0.1:8080", true}, // loopback always allowed + {"[::1]:6379", true}, // IPv6 loopback allowed + {"169.254.169.254:80", false}, // cloud metadata + {"10.0.0.5:443", false}, // RFC1918 + {"172.16.0.1:80", false}, // RFC1918 + {"192.168.1.5:80", false}, // RFC1918 + {"100.64.0.1:80", false}, // CGNAT + {"[fd00::1]:443", false}, // IPv6 ULA + {"[fe80::1]:80", false}, // IPv6 link-local + {"93.184.216.34:443", true}, // public + {"[2001:db8::1]:443", true}, // public IPv6 (example range, unrouted) + } + for _, c := range cases { + got := dialAddressAllowed(c.address, nil) + if got != c.allowed { + t.Errorf("dialAddressAllowed(%q) = %v, want %v", c.address, got, c.allowed) + } + } +} + +// TestWebFetchDialGateHonorsConfigOverrides verifies the gate applies the same +// exemptions as the raw-IP policy: allow_raw_ips and ip_allow_list CIDRs. +func TestWebFetchDialGateHonorsConfigOverrides(t *testing.T) { + t.Parallel() + + rawIPs := &config.Config{ + Tools: config.Tools{ + WebFetch: config.ToolWebFetch{AllowRawIPs: true}, + }, + } + require.True(t, dialAddressAllowed("10.0.0.5:443", rawIPs), + "allow_raw_ips must exempt the dial gate, matching the raw-IP policy") + + cidrCfg := &config.Config{ + Tools: config.Tools{ + WebFetch: config.ToolWebFetch{IPAllowList: []string{"192.168.1.0/24"}}, + }, + } + require.True(t, dialAddressAllowed("192.168.1.5:8080", cidrCfg), + "allow-listed CIDR must pass the dial gate") + require.False(t, dialAddressAllowed("10.0.0.5:8080", cidrCfg), + "addresses outside the allow list must stay denied") +} + +// TestWebFetchDialGateLocalhostHostnameAllowed verifies the default gate keeps +// legitimate local dev flows working: an httptest server on 127.0.0.1 must +// still be reachable through the guarded dialer. +func TestWebFetchDialGateLocalhostHostnameAllowed(t *testing.T) { + // t.Setenv makes the test non-parallel by design. + + // Ensure no env proxy interferes with the gate in either direction. + t.Setenv("HTTP_PROXY", "") + t.Setenv("HTTPS_PROXY", "") + t.Setenv("ALL_PROXY", "") + t.Setenv("http_proxy", "") + t.Setenv("https_proxy", "") + t.Setenv("all_proxy", "") + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + allowList = make(map[string]bool) + transport := egress.NewHTTPTransport() + transport.DialContext = guardedDialer(nil).DialContext + client := &http.Client{ + Timeout: time.Second, + Transport: newSecurityTransport(transport, func(ctx context.Context, host string) (bool, error) { + t.Errorf("allowFn should not be called for localhost, got host %q", host) + return false, nil + }, nil), + } + + resp, err := client.Get(srv.URL) + if err != nil { + t.Fatalf("localhost hostname must pass the dial gate, got: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("expected status OK, got %d", resp.StatusCode) + } +} diff --git a/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md b/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md index f5488d0..e787620 100644 --- a/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md +++ b/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md @@ -173,6 +173,7 @@ guarantees. - **Fail-closed**: no network requests succeed until the agent confirms the domain is trusted - **IP-block policy**: raw IP addresses are rejected for both IPv4 and IPv6 literal forms (e.g., `10.x.x.x`, `192.168.x.x`, `[::1]`, `[fd00::…]`) — FQDNs required +- **Resolved-address validation**: the same IP policy is enforced at dial time on the *resolved* socket address, so an approved hostname whose DNS answer rebinds into a private, link-local, or cloud-metadata range is refused at the socket (DNS-rebinding defense). Loopback stays allowed; `IPAllowList`/`AllowRawIPs` exempt like raw IPs; when an environment proxy is configured the operator's proxy is the egress decision and the strict path remains the egress broker tier - **Opt-in overrides**: `AllowRawIPs` + `IPAllowList` (CIDR) allow raw IPs for local/network development - **Layered trust**: workspace + global allow lists + session allow list + user prompt - **OTel observability**: every allow/deny/event is captured as an OTel span with structured attributes From d86fb2ed8022cda260e3f21667ce8f0a79f490a0 Mon Sep 17 00:00:00 2001 From: Tom Maiaroto Date: Tue, 20 Oct 2026 22:15:03 -0700 Subject: [PATCH 4/7] fix: fall back to the large model when no small model is configured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The small slot was auto-defaulted to the first configured provider in the catalog, which silently routed title generation, summarization, and agentic_fetch to a provider the user never chose. An unset (or invalid) small model now falls back to the configured large model instead. ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- docs/MODELS_AND_PROVIDERS_CONFIG.md | 5 ++ pkg/config/load.go | 51 +++++---------- pkg/config/load_test.go | 64 ++++++++++++++++--- .../content/MODELS_AND_PROVIDERS_CONFIG.md | 5 ++ 4 files changed, 81 insertions(+), 44 deletions(-) diff --git a/docs/MODELS_AND_PROVIDERS_CONFIG.md b/docs/MODELS_AND_PROVIDERS_CONFIG.md index e9eb005..e321c6b 100644 --- a/docs/MODELS_AND_PROVIDERS_CONFIG.md +++ b/docs/MODELS_AND_PROVIDERS_CONFIG.md @@ -199,6 +199,11 @@ The `models` section selects which model to use for different tasks. Phosphor us - **`large`** — Primary model for coding, reasoning, and complex tasks - **`small`** — Fallback model for title generation, quick responses, and sub-agents +If `small` is omitted, it falls back to the `large` model rather than +auto-selecting one from the provider catalog. Every consumer of the small +slot (title generation, summarization, `agentic_fetch`) uses the large model +until you pin `models.small` explicitly. + ### Model Fields | Field | Type | Required | Description | diff --git a/pkg/config/load.go b/pkg/config/load.go index 7383e34..eabe752 100644 --- a/pkg/config/load.go +++ b/pkg/config/load.go @@ -796,20 +796,9 @@ func (c *Config) defaultModelSelection(knownProviders []catwalk.Provider) (large ReasoningEffort: defaultLargeModel.DefaultReasoningEffort, } - defaultSmallModel := c.GetModel(string(p.ID), p.DefaultSmallModelID) - if defaultSmallModel == nil { - slog.Warn("Default small model %s not found for provider %s", p.DefaultSmallModelID, p.ID) - if len(providerConfig.Models) == 0 { - return largeModel, smallModel, fmt.Errorf("default small model %s not found for provider %s", p.DefaultSmallModelID, p.ID) - } - defaultSmallModel = &providerConfig.Models[0] - } - smallModel = SelectedModel{ - Provider: string(p.ID), - Model: defaultSmallModel.ID, - MaxTokens: defaultSmallModel.DefaultMaxTokens, - ReasoningEffort: defaultSmallModel.DefaultReasoningEffort, - } + // The small slot is intentionally left unset: small-model + // consumers fall back to the large model instead of guessing + // one from the catalog. return largeModel, smallModel, err } @@ -923,12 +912,12 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid model := c.GetModel(small.Provider, small.Model) if model == nil { - small = defaultSmall - if persist { - if err := store.UpdatePreferredModel(ScopeGlobal, SelectedModelTypeSmall, small); err != nil { - return fmt.Errorf("failed to update preferred small model: %w", err) - } - } + // The configured small model no longer resolves (provider + // removed or renamed). Fall back to the large model instead + // of persisting a catalog guess. + slog.Warn("Configured small model not found, falling back to large model", + "provider", small.Provider, "model", small.Model) + small = large } else { if smallModelSelected.MaxTokens > 0 { small.MaxTokens = smallModelSelected.MaxTokens @@ -975,22 +964,12 @@ func configureSelectedModels(store *ConfigStore, knownProviders []catwalk.Provid } } - // When small isn't explicitly configured and the provider isn't a - // known built-in, use the large model as the small model. This - // prevents two different models from being requested concurrently - // for local/openai-compat providers. - if !smallModelConfigured { - isKnownProvider := false - for _, kp := range knownProviders { - if string(kp.ID) == small.Provider { - isKnownProvider = true - break - } - } - if !isKnownProvider { - slog.Warn("Using large model as small model for unknown provider", "provider", large.Provider, "model", large.Model) - small = large - } + // When small isn't configured it falls back to the large model + // rather than auto-selecting one from the provider catalog — the + // auto-pick landed on arbitrary providers users never chose. + if _, ok := c.Models[SelectedModelTypeSmall]; !ok { + slog.Info("Small model not configured, falling back to large model", "provider", large.Provider, "model", large.Model) + small = large } c.Models[SelectedModelTypeLarge] = large diff --git a/pkg/config/load_test.go b/pkg/config/load_test.go index 0b3126a..24c2bf6 100644 --- a/pkg/config/load_test.go +++ b/pkg/config/load_test.go @@ -1277,9 +1277,10 @@ func TestConfig_defaultModelSelection(t *testing.T) { require.Equal(t, "large-model", large.Model) require.Equal(t, "openai", large.Provider) require.Equal(t, int64(1000), large.MaxTokens) - require.Equal(t, "small-model", small.Model) - require.Equal(t, "openai", small.Provider) - require.Equal(t, int64(500), small.MaxTokens) + // The small slot is intentionally left unset so consumers fall + // back to the large model instead of guessing one from the catalog. + require.Empty(t, small.Model) + require.Empty(t, small.Provider) }) t.Run("should error if no providers configured", func(t *testing.T) { knownProviders := []catwalk.Provider{ @@ -1471,9 +1472,10 @@ func TestConfig_defaultModelSelection(t *testing.T) { require.Equal(t, "large-model", large.Model) require.Equal(t, "openai", large.Provider) require.Equal(t, int64(1000), large.MaxTokens) - require.Equal(t, "small-model", small.Model) - require.Equal(t, "openai", small.Provider) - require.Equal(t, int64(500), small.MaxTokens) + // The small slot is intentionally left unset so consumers fall + // back to the large model instead of guessing one from the catalog. + require.Empty(t, small.Model) + require.Empty(t, small.Provider) }) } @@ -1776,9 +1778,10 @@ func TestConfig_configureSelectedModels(t *testing.T) { require.Equal(t, "larger-model", large.Model) require.Equal(t, "openai", large.Provider) require.Equal(t, int64(2000), large.MaxTokens) - require.Equal(t, "small-model", small.Model) + // Small is unset, so it falls back to the configured large model. + require.Equal(t, "larger-model", small.Model) require.Equal(t, "openai", small.Provider) - require.Equal(t, int64(500), small.MaxTokens) + require.Equal(t, int64(2000), small.MaxTokens) }) t.Run("should be possible to use multiple providers", func(t *testing.T) { knownProviders := []catwalk.Provider{ @@ -1843,6 +1846,51 @@ func TestConfig_configureSelectedModels(t *testing.T) { require.Equal(t, int64(300), small.MaxTokens) }) + t.Run("should fall back to the large model when the small model is invalid", func(t *testing.T) { + knownProviders := []catwalk.Provider{ + { + ID: "openai", + APIKey: "abc", + DefaultLargeModelID: "large-model", + DefaultSmallModelID: "small-model", + Models: []catwalk.Model{ + { + ID: "large-model", + DefaultMaxTokens: 1000, + }, + { + ID: "small-model", + DefaultMaxTokens: 500, + }, + }, + }, + } + + cfg := &Config{ + Models: map[SelectedModelType]SelectedModel{ + "large": { + Model: "large-model", + Provider: "openai", + }, + "small": { + Model: "ghost-model", + Provider: "ghost-provider", + }, + }, + } + cfg.setDefaults("/tmp", "") + env := env.NewFromMap(map[string]string{}) + resolver := NewShellVariableResolver(env) + err := cfg.configureProviders(context.Background(), testStore(cfg), env, resolver, knownProviders) + require.NoError(t, err) + + err = configureSelectedModels(testStore(cfg), knownProviders, false) + require.NoError(t, err) + small := cfg.Models[SelectedModelTypeSmall] + require.Equal(t, "large-model", small.Model) + require.Equal(t, "openai", small.Provider) + }) + t.Run("should override the max tokens only", func(t *testing.T) { knownProviders := []catwalk.Provider{ { diff --git a/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md b/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md index e9eb005..e321c6b 100644 --- a/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md +++ b/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md @@ -199,6 +199,11 @@ The `models` section selects which model to use for different tasks. Phosphor us - **`large`** — Primary model for coding, reasoning, and complex tasks - **`small`** — Fallback model for title generation, quick responses, and sub-agents +If `small` is omitted, it falls back to the `large` model rather than +auto-selecting one from the provider catalog. Every consumer of the small +slot (title generation, summarization, `agentic_fetch`) uses the large model +until you pin `models.small` explicitly. + ### Model Fields | Field | Type | Required | Description | From 731f2436b794a7ff93dabf16a32b9b1d14b25977 Mon Sep 17 00:00:00 2001 From: Tom Maiaroto Date: Tue, 20 Oct 2026 22:33:28 -0700 Subject: [PATCH 5/7] docs: clarify config-file merge order and the small-model fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Readers can reasonably expect a provider listed in the model dialog to be in use, or wonder where a models.small selection came from when their own config omits it. Spell out the config file precedence and deep-merge behavior, and that an unset small slot falls back to the large model rather than a catalog guess. ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- docs/CONFIGURATION.md | 12 +++++++++++- docs/MODELS_AND_PROVIDERS_CONFIG.md | 13 ++++++++++++- pkg/phosphordocs/content/CONFIGURATION.md | 12 +++++++++++- .../content/MODELS_AND_PROVIDERS_CONFIG.md | 13 ++++++++++++- 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index e26f4b6..50f3bcb 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -25,9 +25,17 @@ order they are loaded (later files win on conflicts): Windows uses the same XDG logic for the global config, so the default is `%USERPROFILE%\.config\phosphor\phosphor.json` unless `XDG_CONFIG_HOME` is set. +All files are **deep-merged per JSON key**, not replaced wholesale — a project +file that sets only `models.large` leaves everything else in your global file +intact. The files with the higher numbers above override the lower ones for +any key they define. + The **global data config** is where Phosphor persists things it learns at runtime (recently used models, per-provider settings saved from the TUI). The **project config** is safe to commit to a repository to share settings with your team. +If the TUI ever writes something to the global data config that you disagree +with (e.g. a `models.small` selection), edit or delete that key there — it +overrides the same key in your global config. The workspace also gets a **data directory** (default `.phosphor/` at the project root, configurable via `options.data_directory` or `--data-dir`). It holds the @@ -256,7 +264,9 @@ The minimum viable cloud setup: ``` `large` is the primary coding/reasoning model; `small` handles title generation -and lightweight sub-tasks. Model entries also accept sampling overrides +and lightweight sub-tasks (summarization, `agentic_fetch`). If `small` is +omitted it falls back to `large` — Phosphor never auto-selects a small model +from another provider. Model entries also accept sampling overrides (`temperature`, `top_p`, `top_k`, `max_tokens`, `seed`, `min_p`, `repetition_penalty`, `stop`, …) plus `reasoning_effort` (OpenAI-style), `think` (Anthropic), and `max_thinking_tokens`. diff --git a/docs/MODELS_AND_PROVIDERS_CONFIG.md b/docs/MODELS_AND_PROVIDERS_CONFIG.md index e321c6b..5e3046b 100644 --- a/docs/MODELS_AND_PROVIDERS_CONFIG.md +++ b/docs/MODELS_AND_PROVIDERS_CONFIG.md @@ -202,7 +202,18 @@ The `models` section selects which model to use for different tasks. Phosphor us If `small` is omitted, it falls back to the `large` model rather than auto-selecting one from the provider catalog. Every consumer of the small slot (title generation, summarization, `agentic_fetch`) uses the large model -until you pin `models.small` explicitly. +until you pin `models.small` explicitly. A `models.small` whose provider or +model no longer resolves also falls back to `large`. + +The list of providers and models offered in the model dialog is **not** +stored in your config: it comes from the [Catwalk](https://catwalk.charm.land) +catalog, fetched at runtime and cached alongside the global data config +(`%LOCALAPPDATA%\phosphor\providers.json` on Windows, +`~/.local/share/phosphor/providers.json` on Linux/macOS; override the source +with `CATWALK_URL`). A known provider appears in the dialog as soon as its +API key is available in your environment — that does **not** mean it is +selected. `phosphor.json` only pins or overrides choices; anything you don't +pin resolves against the catalog. ### Model Fields diff --git a/pkg/phosphordocs/content/CONFIGURATION.md b/pkg/phosphordocs/content/CONFIGURATION.md index e26f4b6..50f3bcb 100644 --- a/pkg/phosphordocs/content/CONFIGURATION.md +++ b/pkg/phosphordocs/content/CONFIGURATION.md @@ -25,9 +25,17 @@ order they are loaded (later files win on conflicts): Windows uses the same XDG logic for the global config, so the default is `%USERPROFILE%\.config\phosphor\phosphor.json` unless `XDG_CONFIG_HOME` is set. +All files are **deep-merged per JSON key**, not replaced wholesale — a project +file that sets only `models.large` leaves everything else in your global file +intact. The files with the higher numbers above override the lower ones for +any key they define. + The **global data config** is where Phosphor persists things it learns at runtime (recently used models, per-provider settings saved from the TUI). The **project config** is safe to commit to a repository to share settings with your team. +If the TUI ever writes something to the global data config that you disagree +with (e.g. a `models.small` selection), edit or delete that key there — it +overrides the same key in your global config. The workspace also gets a **data directory** (default `.phosphor/` at the project root, configurable via `options.data_directory` or `--data-dir`). It holds the @@ -256,7 +264,9 @@ The minimum viable cloud setup: ``` `large` is the primary coding/reasoning model; `small` handles title generation -and lightweight sub-tasks. Model entries also accept sampling overrides +and lightweight sub-tasks (summarization, `agentic_fetch`). If `small` is +omitted it falls back to `large` — Phosphor never auto-selects a small model +from another provider. Model entries also accept sampling overrides (`temperature`, `top_p`, `top_k`, `max_tokens`, `seed`, `min_p`, `repetition_penalty`, `stop`, …) plus `reasoning_effort` (OpenAI-style), `think` (Anthropic), and `max_thinking_tokens`. diff --git a/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md b/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md index e321c6b..5e3046b 100644 --- a/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md +++ b/pkg/phosphordocs/content/MODELS_AND_PROVIDERS_CONFIG.md @@ -202,7 +202,18 @@ The `models` section selects which model to use for different tasks. Phosphor us If `small` is omitted, it falls back to the `large` model rather than auto-selecting one from the provider catalog. Every consumer of the small slot (title generation, summarization, `agentic_fetch`) uses the large model -until you pin `models.small` explicitly. +until you pin `models.small` explicitly. A `models.small` whose provider or +model no longer resolves also falls back to `large`. + +The list of providers and models offered in the model dialog is **not** +stored in your config: it comes from the [Catwalk](https://catwalk.charm.land) +catalog, fetched at runtime and cached alongside the global data config +(`%LOCALAPPDATA%\phosphor\providers.json` on Windows, +`~/.local/share/phosphor/providers.json` on Linux/macOS; override the source +with `CATWALK_URL`). A known provider appears in the dialog as soon as its +API key is available in your environment — that does **not** mean it is +selected. `phosphor.json` only pins or overrides choices; anything you don't +pin resolves against the catalog. ### Model Fields From e80ebef42a16c46fc5aa5e323d532bae1f8f36af Mon Sep 17 00:00:00 2001 From: Tom Maiaroto Date: Tue, 20 Oct 2026 23:28:35 -0700 Subject: [PATCH 6/7] fix: close CodeRabbit review gaps in wire redaction and web-fetch dial guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- pkg/agent/outgoing_redact.go | 30 ++++++- pkg/agent/tools/web_fetch.go | 128 +++++++++++++++++++++++------- pkg/agent/tools/web_fetch_test.go | 20 ++--- pkg/egress/transport.go | 62 ++++++++++++--- 4 files changed, 186 insertions(+), 54 deletions(-) diff --git a/pkg/agent/outgoing_redact.go b/pkg/agent/outgoing_redact.go index 1a5930b..7236e4c 100644 --- a/pkg/agent/outgoing_redact.go +++ b/pkg/agent/outgoing_redact.go @@ -1,6 +1,7 @@ package agent import ( + "errors" "log/slog" "regexp" "strings" @@ -171,13 +172,34 @@ func (a *sessionAgent) redactOutgoingMessages(msgs []fantasy.Message) []fantasy. case fantasy.ToolResultPart: // Text results are normally masked per-tool, but anything // that reaches a result outside the redacted surfaces must - // still meet the last-resort mask at the wire. - if txt, ok := fantasy.AsToolResultOutputType[fantasy.ToolResultOutputContentText](p.Output); ok { - next := a.redactOutgoingText(txt.Text) - if next != txt.Text { + // still meet the last-resort mask at the wire. Error text + // and media-attached text ride the wire verbatim too. + switch out := p.Output.(type) { + case fantasy.ToolResultOutputContentText: + next := a.redactOutgoingText(out.Text) + if next != out.Text { p.Output = fantasy.ToolResultOutputContentText{Text: next} changed = true } + case fantasy.ToolResultOutputContentError: + if out.Error == nil { + break + } + next := a.redactOutgoingText(out.Error.Error()) + if next != out.Error.Error() { + p.Output = fantasy.ToolResultOutputContentError{Error: errors.New(next)} + changed = true + } + case fantasy.ToolResultOutputContentMedia: + if out.Text == "" { + break + } + next := a.redactOutgoingText(out.Text) + if next != out.Text { + out.Text = next + p.Output = out + changed = true + } } parts[j] = p default: diff --git a/pkg/agent/tools/web_fetch.go b/pkg/agent/tools/web_fetch.go index d620165..09ba918 100644 --- a/pkg/agent/tools/web_fetch.go +++ b/pkg/agent/tools/web_fetch.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strings" + "sync" "syscall" "time" @@ -35,8 +36,34 @@ var webFetchDescriptionTpl = template.Must( ) // allowList is the in-memory cache of domains that have been explicitly approved. -// Session-local: cleared when the user's session restarts. -var allowList = map[string]bool{} +// Session-local: cleared when the user's session restarts. Guarded by +// allowListMu — web_fetch runs concurrently in parallel tool batches. +var ( + allowListMu sync.RWMutex + allowList = map[string]bool{} +) + +// hostInAllowList reports whether host was approved earlier in this session. +func hostInAllowList(host string) bool { + allowListMu.RLock() + defer allowListMu.RUnlock() + return allowList[host] +} + +// markHostAllowed records host as approved for the rest of the session. +func markHostAllowed(host string) { + allowListMu.Lock() + defer allowListMu.Unlock() + allowList[host] = true +} + +// resetAllowList clears the session allow list. Used by tests that need a +// known-clean trust state. +func resetAllowList() { + allowListMu.Lock() + defer allowListMu.Unlock() + allowList = map[string]bool{} +} // securityTransport is a custom http.RoundTripper that intercepts each outgoing // request, validates the destination, and applies a layered trust model before @@ -138,7 +165,7 @@ func (t *securityTransport) RoundTrip(req *http.Request) (*http.Response, error) return nil, fmt.Errorf("web_fetch request to %s is blocked by user policy", lookupHost) } // User approved — write to the in-memory allow list so we don't re-prompt. - allowList[lookupHost] = true + markHostAllowed(lookupHost) } // Tag with allow info before the actual network call. @@ -162,7 +189,7 @@ func recordAllowEvent(span trace.Span, source string) { // allow list, and global-level user allow list. Returns true if any source allows it. func (t *securityTransport) isAllowed(host string, span trace.Span) bool { // 1. In-memory allow list (session-level, populated by user prompt). - if allowList[host] { + if hostInAllowList(host) { recordAllowEvent(span, "session_allow_list") return true } @@ -336,21 +363,14 @@ var webFetchDeniedCIDRs = []string{ "FE80::/10", // IPv6 link-local } -// envProxyConfigured reports whether the environment names an HTTP(S) proxy. -// When a proxy is set, the address web_fetch dials is the proxy's, not the -// target's, and private proxy infrastructure is the operator's own network -// decision; the egress broker tier remains the strict path for that case. -func envProxyConfigured() bool { - for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY"} { - if v := os.Getenv(k); v != "" { - return true - } - if v := os.Getenv(strings.ToLower(k)); v != "" { - return true - } - } - return false -} +// proxyForRequest reports the proxy decision for the request the guarded +// dialer is about to dial. The dial target of a proxied request is the +// proxy's address, not the target's, and private proxy infrastructure is +// the operator's own network decision; the egress broker tier remains the +// strict path for that case. The decision must be per-request: a global +// "any proxy env var is set" check would also exempt direct connections +// selected by NO_PROXY, silently disarming the DNS-rebinding defense. +type proxyForRequest func(*http.Request) (*url.URL, error) // dialAddressAllowed validates the *resolved* socket address of a web_fetch // dial. The trust hierarchy judges hostnames as strings; this gate judges the @@ -379,6 +399,12 @@ func dialAddressAllowed(address string, cfg *config.Config) bool { return true } for _, cidr := range cfg.Tools.WebFetch.IPAllowList { + if lit := net.ParseIP(cidr); lit != nil { + if lit.Equal(ip) { + return true + } + continue + } if matchCIDR(cidr, ip.String()) { return true } @@ -396,11 +422,17 @@ func dialAddressAllowed(address string, cfg *config.Config) bool { // socket address of every web_fetch connection. ControlContext runs after the // kernel has dialed but before any data flows, so the address judged is the // address actually used — there is no re-resolution race to exploit. -func guardedDialer(cfg *config.Config) *net.Dialer { +// proxyCheck reports the effective proxy decision for the current request; +// when a request is routed through a proxy the guard exempts the dial +// (the proxy, not the target, is the socket address). +func guardedDialer(cfg *config.Config, proxyCheck proxyForRequest) *net.Dialer { return &net.Dialer{ - ControlContext: func(_ context.Context, _, address string, _ syscall.RawConn) error { - if envProxyConfigured() { - return nil + ControlContext: func(ctx context.Context, _, address string, _ syscall.RawConn) error { + if proxyCheck != nil { + proxy, err := proxyCheck(ctxproxyReq(ctx)) + if err == nil && proxy != nil { + return nil + } } if !dialAddressAllowed(address, cfg) { host, _, _ := net.SplitHostPort(address) @@ -415,12 +447,54 @@ func guardedDialer(cfg *config.Config) *net.Dialer { } } +// ctxproxyReq returns the request that triggered the dial so the transport's +// Proxy function can decide. The ControlContext callback carries no request, +// so [proxyStash] places it in the context one layer above the dialer. +func ctxproxyReq(ctx context.Context) *http.Request { + if req, ok := ctx.Value(ctxProxyRequest{}).(*http.Request); ok && req != nil { + return req + } + // No request available: treat the connection as direct. + return &http.Request{URL: &url.URL{Scheme: "https", Host: "direct.invalid"}} +} + +type ctxProxyRequest struct{} + +// proxyStash carries the current request into the dial context so the +// guarded dialer's ControlContext can consult the transport's per-request +// proxy decision. http.Transport dials with the request's context, so +// stashing here is enough for the guard to see it. +type proxyStash struct{ base http.RoundTripper } + +func (s proxyStash) RoundTrip(r *http.Request) (*http.Response, error) { + ctx := context.WithValue(r.Context(), ctxProxyRequest{}, r) + return s.base.RoundTrip(r.WithContext(ctx)) +} + // NewWebFetchTool constructs a web fetch tool that uses the security transport // and invokes the TUI permission prompt when a host is not yet approved. func NewWebFetchTool(workingDir string, client *http.Client, cfg *config.Config, allowFn func(ctx context.Context, host string) (bool, error)) fantasy.AgentTool { baseTransport := egress.NewHTTPTransport() - baseTransport.DialContext = guardedDialer(cfg).DialContext - secure := newSecurityTransport(egress.Transport(baseTransport), allowFn, cfg) + guarded := guardedDialer(cfg, baseTransport.Proxy) + baseTransport.DialContext = guarded.DialContext + // The guard must survive both egress paths: the inactive path dials + // through baseTransport (guarded above), the active path replaces it + // with the broker's direct transport, which needs the guard composed + // in explicitly. + guard := func(_ context.Context, _, address string) error { + if !dialAddressAllowed(address, cfg) { + host, _, _ := net.SplitHostPort(address) + return fmt.Errorf( + "web_fetch request to %s blocked (resolved address is in a denied range; DNS rebinding defense)", + host, + ) + } + return nil + } + secure := newSecurityTransport( + egress.Transport(proxyStash{baseTransport}, egress.WithDialGuard(guard)), + allowFn, cfg, + ) secureClient := &http.Client{ Timeout: 30 * time.Second, Transport: secure, @@ -507,7 +581,7 @@ func FetchURLAndConvertWithTrust(ctx context.Context, client *http.Client, rawUR } host := stripPort(parsed.Host) - if !allowList[host] && !isAllowedByGlobal(host) && !isAllowedByWorkspace(host) { + if !hostInAllowList(host) && !isAllowedByGlobal(host) && !isAllowedByWorkspace(host) { allowed, err := allowFn(ctx, host) if err != nil { return "", fmt.Errorf("host %s not approved by user prompt: %w", host, err) @@ -515,7 +589,7 @@ func FetchURLAndConvertWithTrust(ctx context.Context, client *http.Client, rawUR if !allowed { return "", fmt.Errorf("host %s blocked by user policy", host) } - allowList[host] = true + markHostAllowed(host) } span.SetAttributes( diff --git a/pkg/agent/tools/web_fetch_test.go b/pkg/agent/tools/web_fetch_test.go index 3292dbd..5918fa0 100644 --- a/pkg/agent/tools/web_fetch_test.go +++ b/pkg/agent/tools/web_fetch_test.go @@ -17,7 +17,7 @@ import ( func TestWebFetchHardeningIPBlock(t *testing.T) { t.Parallel() - allowList = make(map[string]bool) + resetAllowList() client := &http.Client{Timeout: time.Second} tr := newSecurityTransport(http.DefaultTransport, func(ctx context.Context, host string) (bool, error) { @@ -46,7 +46,7 @@ func TestWebFetchHardeningIPBlock(t *testing.T) { func TestWebFetchHardeningCIDRAllow(t *testing.T) { t.Parallel() - allowList = make(map[string]bool) + resetAllowList() cfg := &config.Config{ Tools: config.Tools{ WebFetch: config.ToolWebFetch{ @@ -88,7 +88,7 @@ func TestWebFetchHardeningLocalhostAlwaysAllowed(t *testing.T) { })) defer ts.Close() - allowList = make(map[string]bool) + resetAllowList() tr := newSecurityTransport(http.DefaultTransport, func(ctx context.Context, host string) (bool, error) { t.Errorf("allowFn should not be called for localhost") return false, nil @@ -119,7 +119,7 @@ func TestWebFetchHardeningLocalhostAlwaysAllowed(t *testing.T) { func TestWebFetchHardeningUserPromptVerified(t *testing.T) { t.Parallel() - allowList = make(map[string]bool) + resetAllowList() called := false tr := newSecurityTransport(http.DefaultTransport, func(ctx context.Context, host string) (bool, error) { called = true @@ -152,7 +152,7 @@ func TestWebFetchHardeningUserPromptVerified(t *testing.T) { func TestWebFetchHardeningUserPromptDenyVerified(t *testing.T) { t.Parallel() - allowList = make(map[string]bool) + resetAllowList() called := false tr := newSecurityTransport(http.DefaultTransport, func(ctx context.Context, host string) (bool, error) { called = true @@ -184,9 +184,6 @@ func TestWebFetchHardeningUserPromptDenyVerified(t *testing.T) { // TestWebFetchHardeningIPv6LiteralBlocked demonstrates bracketed IPv6 literals // are treated as raw IPs and fail closed without reaching the trust hierarchy. func TestWebFetchHardeningIPv6LiteralBlocked(t *testing.T) { - t.Parallel() - - allowList = make(map[string]bool) client := &http.Client{Timeout: time.Second} tr := newSecurityTransport(http.DefaultTransport, func(ctx context.Context, host string) (bool, error) { @@ -221,9 +218,6 @@ func TestWebFetchHardeningIPv6LiteralBlocked(t *testing.T) { // TestWebFetchHardeningIPv6LocalhostAllowed demonstrates [::1] is treated as // localhost and allowed through without a prompt. func TestWebFetchHardeningIPv6LocalhostAllowed(t *testing.T) { - t.Parallel() - - allowList = make(map[string]bool) tr := newSecurityTransport(http.DefaultTransport, func(ctx context.Context, host string) (bool, error) { t.Errorf("allowFn should not be called for IPv6 localhost") return false, nil @@ -323,9 +317,9 @@ func TestWebFetchDialGateLocalhostHostnameAllowed(t *testing.T) { })) defer srv.Close() - allowList = make(map[string]bool) + resetAllowList() transport := egress.NewHTTPTransport() - transport.DialContext = guardedDialer(nil).DialContext + transport.DialContext = guardedDialer(nil, transport.Proxy).DialContext client := &http.Client{ Timeout: time.Second, Transport: newSecurityTransport(transport, func(ctx context.Context, host string) (bool, error) { diff --git a/pkg/egress/transport.go b/pkg/egress/transport.go index c215e3f..ee37879 100644 --- a/pkg/egress/transport.go +++ b/pkg/egress/transport.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "net/url" + "syscall" "time" ) @@ -58,32 +59,72 @@ func envProxyWhenBrokerInactive(r *http.Request) (*url.URL, error) { return http.ProxyFromEnvironment(r) } +// TransportOption customizes the RoundTripper returned by [Transport]. +type TransportOption func(*RoundTripperPolicy) + +// WithDialGuard composes guard into the resolved-address checks of the direct +// transport used while the broker is active. Without it, wrapping a guarded +// base transport would silently drop the caller's dial gate on the active +// path, because the active path replaces the base transport. +func WithDialGuard(guard func(ctx context.Context, network, address string) error) TransportOption { + return func(rt *RoundTripperPolicy) { + rt.dialGuard = guard + } +} + // Transport wraps base with the broker-aware policy and sentinel resolver. A nil // base uses a broker-aware clone of http.DefaultTransport. The wrapper consults // the frozen broker state on each request, so a client constructed before the // coordinator arms the tier still honors it after [Start]. -func Transport(base http.RoundTripper) http.RoundTripper { +func Transport(base http.RoundTripper, opts ...TransportOption) http.RoundTripper { if base == nil { base = NewHTTPTransport() } - return &RoundTripperPolicy{ + rt := &RoundTripperPolicy{ store: Store(), inner: base, direct: newDirectTransport(), policyFunc: brokerPolicy, } + for _, opt := range opts { + opt(rt) + } + if rt.dialGuard != nil { + if tr, ok := rt.direct.(*http.Transport); ok { + tr.DialContext = dialPolicyAwareGuarded(rt.dialGuard) + } + } + return rt } func dialPolicyAware(ctx context.Context, network, address string) (net.Conn, error) { - policy, active, failed := brokerPolicy() - if failed { - return nil, ErrBrokerFailedToStart - } - dialer := policy.Dialer() - if !active { - dialer = &net.Dialer{KeepAlive: DefaultKeepAlive} + return dialPolicyAwareGuarded(nil)(ctx, network, address) +} + +func dialPolicyAwareGuarded(guard func(ctx context.Context, network, address string) error) func(context.Context, string, string) (net.Conn, error) { + return func(ctx context.Context, network, address string) (net.Conn, error) { + policy, active, failed := brokerPolicy() + if failed { + return nil, ErrBrokerFailedToStart + } + dialer := &net.Dialer{KeepAlive: DefaultKeepAlive} + if active { + dialer.ControlContext = func(ctx context.Context, network, address string, c syscall.RawConn) error { + if err := policy.ControlContext(ctx, network, address, c); err != nil { + return err + } + if guard != nil { + return guard(ctx, network, address) + } + return nil + } + } else if guard != nil { + dialer.ControlContext = func(ctx context.Context, network, address string, _ syscall.RawConn) error { + return guard(ctx, network, address) + } + } + return dialer.DialContext(ctx, network, address) } - return dialer.DialContext(ctx, network, address) } // RoundTripperPolicy is an [http.RoundTripper] that enforces the net-policy and @@ -95,6 +136,7 @@ type RoundTripperPolicy struct { store TokenResolver inner http.RoundTripper direct http.RoundTripper + dialGuard func(ctx context.Context, network, address string) error } // NewRoundTripper builds a broker transport over inner (nil means a broker-aware From b0ee274efdc48d9a8b97e58a4ac314a816d26b6b Mon Sep 17 00:00:00 2001 From: Tom Maiaroto Date: Wed, 21 Oct 2026 06:40:58 -0700 Subject: [PATCH 7/7] docs: match raw-IP egress docs to the fail-closed behavior in code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ஃ Generated with Phosphor Assisted-by: Phosphor:CYBER-FROST-3.8-EXL3-SAGE-3.87bpw --- docs/security/NETWORK_EGRESS_HARDENING.md | 23 +++++++++---------- .../security/NETWORK_EGRESS_HARDENING.md | 23 +++++++++---------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/docs/security/NETWORK_EGRESS_HARDENING.md b/docs/security/NETWORK_EGRESS_HARDENING.md index e787620..a53842a 100644 --- a/docs/security/NETWORK_EGRESS_HARDENING.md +++ b/docs/security/NETWORK_EGRESS_HARDENING.md @@ -31,7 +31,7 @@ Phosphor's web-fetch and web-search tools now operate under an **Adaptive Trust* ## Components -### `internal/agent/tools/web_fetch.go` +### `pkg/agent/tools/web_fetch.go` - `securityTransport` — custom `http.RoundTripper` that intercepts each request - `allowList` — in-memory session allow cache (cleared on session restart) @@ -42,7 +42,7 @@ Phosphor's web-fetch and web-search tools now operate under an **Adaptive Trust* - `FetchURLAndConvertWithTrust` — wraps `FetchURLAndConvert` with the trust check - `NewWebFetchTool` — constructs tool with `allowFn` callback (TUI permission prompt) -### `internal/agent/tools/web_search.go` +### `pkg/agent/tools/web_search.go` - `NewWebSearchTool` — same security contract as `web_fetch`; panics if `client` is nil - `sanitizeSearchResults` — regex scrubber redacts: @@ -51,7 +51,7 @@ Phosphor's web-fetch and web-search tools now operate under an **Adaptive Trust* - `randomDelay` — adds 100–150ms random delay per round-trip to stay under DDG's request-budget threshold - OTel span tagged with `attribute.Int("gen_ai.tool.web_search.results", len(results))` -### `internal/agent/coordinator.go` +### `pkg/agent/coordinator.go` - `tuiAllowPrompt` field added to `coordinator` struct - Wired in `NewCoordinator` via `c.permissions.Request` with synthetic tool-call ID (`tui:allow:`) @@ -101,8 +101,8 @@ Phosphor's `web_fetch` and `web_search` tools consult the user's `phosphor.json` | Field | Type | Default | Description | |---|---|---|---| -| `IPAllowList` | `[]string` | `[]` | CIDR or literal IPs allowed to be reached | -| `AllowRawIPs` | `bool` | `false` | Opt-in for raw IP access (default: FQDN required) | +| `IPAllowList` | `[]string` | `[]` | CIDR or literal IPs allowed to be reached (consulted only when `allow_raw_ips` is true) | +| `AllowRawIPs` | `bool` | `false` | Gate for raw IP access; IPs are allowed only if they also match `IPAllowList` (default: FQDN required) | ### Example `phosphor.json` @@ -126,9 +126,8 @@ Phosphor's `web_fetch` and `web_search` tools consult the user's `phosphor.json` When `securityTransport` intercepts a request to a raw IP: 1. **127.0.0.1 / ::1** (localhost) → always allowed, no config needed -2. **`IPAllowList`** → literal match or CIDR match via `matchCIDR` → allowed -3. **`AllowRawIPs = true`** → allowed (if no CIDR match) -4. **No match** → TUI prompt shows "WebFetch is requesting access to [host]. Allow this domain?" +2. **`AllowRawIPs = true` + `IPAllowList`** → literal match or CIDR match via `matchCIDR` → allowed +3. **Anything else** → the request fails closed with an error; raw IPs never reach the TUI prompt (prompts apply to hostnames only) ## Bash Network Egress Policy @@ -208,10 +207,10 @@ way they would for a browser. ## Files Modified -- `internal/agent/tools/web_fetch.go` — added security transport, allow lists, TUI prompt wiring -- `internal/agent/tools/web_search.go` — added sanitization, OTel tagging, random delay, nil client panic -- `internal/agent/coordinator.go` — added `tuiAllowPrompt` field and wiring -- `internal/agent/agentic_fetch_tool.go` — updated call site to pass `c.tuiAllowPrompt` +- `pkg/agent/tools/web_fetch.go` — added security transport, allow lists, TUI prompt wiring +- `pkg/agent/tools/web_search.go` — added sanitization, OTel tagging, random delay, nil client panic +- `pkg/agent/coordinator.go` — added `tuiAllowPrompt` field and wiring +- `pkg/agent/agentic_fetch_tool.go` — updated call site to pass `c.tuiAllowPrompt` ## Notes diff --git a/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md b/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md index e787620..a53842a 100644 --- a/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md +++ b/pkg/phosphordocs/content/security/NETWORK_EGRESS_HARDENING.md @@ -31,7 +31,7 @@ Phosphor's web-fetch and web-search tools now operate under an **Adaptive Trust* ## Components -### `internal/agent/tools/web_fetch.go` +### `pkg/agent/tools/web_fetch.go` - `securityTransport` — custom `http.RoundTripper` that intercepts each request - `allowList` — in-memory session allow cache (cleared on session restart) @@ -42,7 +42,7 @@ Phosphor's web-fetch and web-search tools now operate under an **Adaptive Trust* - `FetchURLAndConvertWithTrust` — wraps `FetchURLAndConvert` with the trust check - `NewWebFetchTool` — constructs tool with `allowFn` callback (TUI permission prompt) -### `internal/agent/tools/web_search.go` +### `pkg/agent/tools/web_search.go` - `NewWebSearchTool` — same security contract as `web_fetch`; panics if `client` is nil - `sanitizeSearchResults` — regex scrubber redacts: @@ -51,7 +51,7 @@ Phosphor's web-fetch and web-search tools now operate under an **Adaptive Trust* - `randomDelay` — adds 100–150ms random delay per round-trip to stay under DDG's request-budget threshold - OTel span tagged with `attribute.Int("gen_ai.tool.web_search.results", len(results))` -### `internal/agent/coordinator.go` +### `pkg/agent/coordinator.go` - `tuiAllowPrompt` field added to `coordinator` struct - Wired in `NewCoordinator` via `c.permissions.Request` with synthetic tool-call ID (`tui:allow:`) @@ -101,8 +101,8 @@ Phosphor's `web_fetch` and `web_search` tools consult the user's `phosphor.json` | Field | Type | Default | Description | |---|---|---|---| -| `IPAllowList` | `[]string` | `[]` | CIDR or literal IPs allowed to be reached | -| `AllowRawIPs` | `bool` | `false` | Opt-in for raw IP access (default: FQDN required) | +| `IPAllowList` | `[]string` | `[]` | CIDR or literal IPs allowed to be reached (consulted only when `allow_raw_ips` is true) | +| `AllowRawIPs` | `bool` | `false` | Gate for raw IP access; IPs are allowed only if they also match `IPAllowList` (default: FQDN required) | ### Example `phosphor.json` @@ -126,9 +126,8 @@ Phosphor's `web_fetch` and `web_search` tools consult the user's `phosphor.json` When `securityTransport` intercepts a request to a raw IP: 1. **127.0.0.1 / ::1** (localhost) → always allowed, no config needed -2. **`IPAllowList`** → literal match or CIDR match via `matchCIDR` → allowed -3. **`AllowRawIPs = true`** → allowed (if no CIDR match) -4. **No match** → TUI prompt shows "WebFetch is requesting access to [host]. Allow this domain?" +2. **`AllowRawIPs = true` + `IPAllowList`** → literal match or CIDR match via `matchCIDR` → allowed +3. **Anything else** → the request fails closed with an error; raw IPs never reach the TUI prompt (prompts apply to hostnames only) ## Bash Network Egress Policy @@ -208,10 +207,10 @@ way they would for a browser. ## Files Modified -- `internal/agent/tools/web_fetch.go` — added security transport, allow lists, TUI prompt wiring -- `internal/agent/tools/web_search.go` — added sanitization, OTel tagging, random delay, nil client panic -- `internal/agent/coordinator.go` — added `tuiAllowPrompt` field and wiring -- `internal/agent/agentic_fetch_tool.go` — updated call site to pass `c.tuiAllowPrompt` +- `pkg/agent/tools/web_fetch.go` — added security transport, allow lists, TUI prompt wiring +- `pkg/agent/tools/web_search.go` — added sanitization, OTel tagging, random delay, nil client panic +- `pkg/agent/coordinator.go` — added `tuiAllowPrompt` field and wiring +- `pkg/agent/agentic_fetch_tool.go` — updated call site to pass `c.tuiAllowPrompt` ## Notes