From 3f2b159ca1b6a270d3da9627e58af6488dc36dfb Mon Sep 17 00:00:00 2001 From: vikingowl Date: Tue, 19 May 2026 22:28:46 +0200 Subject: [PATCH 1/3] feat(security): add SafeProvider boundary wrapper (W1-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces internal/security/SafeProvider — a provider.Provider decorator that scans outgoing messages and the system prompt through the firewall before delegating to the inner provider. Tool-result redaction stays in the engine because it needs per-tool context the boundary lacks. FirewallRef provides a late-binding atomic.Pointer[Firewall] so the wrapper can be installed before NewFirewall runs in main. A nil or unset ref makes SafeProvider a pass-through — preserves the current init order without lock contention or panics. Wave 1 of the post-audit hardening plan (docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md). Closes the architectural critique that secret scanning only ran inside engine.buildRequest(), leaving SLM/summarizer/hook/routerStreamer paths to send raw payloads. This commit only ships the wrapper; W1-2 and W1-3 will wire it through main and the four bypass sites. --- .../2026-05-19-security-wave1-safeprovider.md | 265 ++++++++++++++++++ internal/security/safeprovider.go | 66 +++++ internal/security/safeprovider_test.go | 243 ++++++++++++++++ internal/security/saferef.go | 28 ++ 4 files changed, 602 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md create mode 100644 internal/security/safeprovider.go create mode 100644 internal/security/safeprovider_test.go create mode 100644 internal/security/saferef.go diff --git a/docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md b/docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md new file mode 100644 index 0000000..9c8af18 --- /dev/null +++ b/docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md @@ -0,0 +1,265 @@ +# Security Hardening Wave 1 — SafeProvider Boundary — 2026-05-19 + +Addresses findings 1–4 of the 2026-05-19 external audit +(`docs/audits/2026-05-19-main2-firewall-audit.md`, if archived; otherwise +the audit transcript in conversation). The audit's central architectural +critique is that the `Firewall` is wired at call sites (only inside +`engine.buildRequest()`), not at the provider boundary. As a result, +every non-engine consumer of a `provider.Provider` skips outgoing +redaction. + +Verified non-engine consumers that today send raw payloads: + +- `internal/slm/classifier.go:105` — sends raw user prompt to the SLM + provider. Risk depends on backend (low for `ollama`/`llamafile`, + real for `openaicompat` against a remote endpoint). +- `internal/context/summarize.go:109` — sends conversation history to + the primary provider. Tool outputs in history are already redacted + (`loop.go:706`), but user input and assistant prose are not. +- `cmd/gnoma/main.go:1225` (`routerStreamer`) — `/init` and similar + flows build a raw request and call `rs.router.Stream(...)`. +- `internal/hook/prompt.go:80` — prompt-type hooks call the streamer + directly with hook-supplied prompts. + +Inside the main engine loop, `Router.Stream` and `prov.Stream` calls +at `loop.go:127, 158, 191, 206, 769, 772` consume requests already +scanned in `buildRequest()` — they're fine, just listed by the audit +for completeness. + +This wave is scoped to closing the bypass at the provider boundary. +Incognito coherence (Wave 2) and scanner/path hygiene (Wave 3) are +separate plan docs. + +--- + +## Approach + +Wrap every `provider.Provider` with a decorator (`security.SafeProvider`) +that runs outgoing-message and system-prompt redaction before delegating +to the inner provider. Apply the wrapper at every registration boundary: + +- primary provider construction in `cmd/gnoma/main.go` +- router arm `Provider` field (initial registration + discovery + factory + CLI agent registration) +- SLM `Classifier.provider` construction +- summarizer `SummarizeStrategy.Provider` construction +- hook streamer construction + +Tool-result redaction stays in the engine (it depends on per-tool +context — origin tool name for logging, etc., which the engine has and +the provider boundary doesn't). The engine's existing scan at +`loop.go:704-707` is unchanged. + +### The construction-order problem + +Today `security.NewFirewall()` runs at `main.go:509`, after every +provider arm has been registered (lines 404, 421-427, 436). A naive +"wrap at registration" implementation would require pulling Firewall +construction earlier, which entangles config validation, +custom-pattern wiring, and incognito state initialization. + +**Solution:** `SafeProvider` accepts a `*FirewallRef` — a tiny +indirection holding an `atomic.Pointer[Firewall]`. The wrapper checks +the pointer on each call; if nil, it delegates without scanning +(matching today's behavior for early-init code paths). Firewall is +installed into the ref once it's constructed. Late-binding keeps the +init order intact and stays goroutine-safe without locking. + +```go +type FirewallRef struct { + p atomic.Pointer[Firewall] +} + +func (r *FirewallRef) Set(fw *Firewall) { r.p.Store(fw) } +func (r *FirewallRef) Get() *Firewall { return r.p.Load() } +``` + +### SafeProvider sketch + +```go +type SafeProvider struct { + inner provider.Provider + fwRef *FirewallRef +} + +func WrapProvider(inner provider.Provider, ref *FirewallRef) *SafeProvider { + return &SafeProvider{inner: inner, fwRef: ref} +} + +func (p *SafeProvider) Stream(ctx context.Context, req provider.Request) (stream.Stream, error) { + if fw := p.fwRef.Get(); fw != nil { + req.Messages = fw.ScanOutgoingMessages(req.Messages) + req.SystemPrompt = fw.ScanSystemPrompt(req.SystemPrompt) + } + return p.inner.Stream(ctx, req) +} + +func (p *SafeProvider) Name() string { return p.inner.Name() } +func (p *SafeProvider) DefaultModel() string { return p.inner.DefaultModel() } +func (p *SafeProvider) Models(ctx context.Context) ([]provider.ModelInfo, error) { + return p.inner.Models(ctx) +} +``` + +`SafeProvider` lives in `internal/security/` because it consumes +`*Firewall`. Importing `security` from `provider` would create a +cycle; the wrapper lives below both. Engine consumers always already +import `security`. + +### Engine reconciliation + +Once `SafeProvider` runs on every arm, `engine.buildRequest()` does +redundant scanning. Options: + +1. **Leave it.** Two scans cost the same as one in steady state + (redaction is idempotent — second pass finds nothing). Slight + defensive belt-and-suspenders. +2. **Remove it.** Simpler. The decorator becomes the sole boundary. + Engine tests need updating; some currently rely on Firewall being + on `engine.Config`. + +**Recommendation: leave it for one release**, then revisit after we've +seen real telemetry that nothing regresses. The cost is two regex +passes per message; not material vs. provider latency. + +--- + +## Tasks + +### W1-1 — SafeProvider + FirewallRef + +- [ ] `internal/security/saferef.go` — `FirewallRef` with + `atomic.Pointer[Firewall]` semantics. +- [ ] `internal/security/safeprovider.go` — `SafeProvider` decorator + implementing `provider.Provider`. Wraps `Stream`; delegates the rest. +- [ ] Unit tests in `internal/security/safeprovider_test.go`: + - ref unset → delegates without scanning (recording fake provider + asserts unmodified request) + - ref set → outgoing messages and system prompt scanned + - tool-call args with embedded API key are redacted on outgoing + path (already covered by `ScanOutgoingMessages` but verify via + the wrapper) + - `Name()`, `Models()`, `DefaultModel()` pass through unchanged + +### W1-2 — Wire ref into main + +- [ ] `cmd/gnoma/main.go` — construct `security.FirewallRef` early, + before any provider construction. Pass to every site that builds + a `provider.Provider`. +- [ ] Existing `fw := security.NewFirewall(...)` call site stays where + it is; immediately call `fwRef.Set(fw)` after construction. + +### W1-3 — Apply to all provider sites + +- [ ] Primary provider: wrap return from `createProvider()` (or wrap + at the call site in `main.go` around line 395 — `armProvider`). +- [ ] Discovered local models: the factory in + `router.RegisterDiscoveredModels(...)` at `main.go:421-427` returns + the inner provider; wrap before returning. +- [ ] Background-discovery factory at `main.go:479-485` — same. +- [ ] CLI agents: `subprocprov.New(agent)` at `main.go:438` — wrap + before passing to `RegisterArm`. +- [ ] SLM classifier: wrap the provider passed into + `slm.NewClassifier(...)`. Site is wherever the SLM manager builds + the classifier (likely `internal/slm/manager.go` or + `cmd/gnoma/main.go`). +- [ ] Summarizer: wrap the provider passed into + `gnomactx.NewSummarizeStrategy(...)` at `main.go:703`. +- [ ] Hook streamer: wrap the provider passed into the hook system + for prompt-type hooks (origin site in `cmd/gnoma/main.go` near + router init). +- [ ] `routerStreamer`: doesn't take a `provider.Provider` directly — + it takes a `*router.Router`. Once all arms are wrapped, this site + inherits the fix. Verify by reading `Router.Stream`'s call into + `decision.Arm.Provider.Stream(...)` at `internal/router/router.go:319`. + +### W1-4 — Tests + +- [ ] Integration test: `internal/security/safeprovider_test.go` adds + a recording fake `Provider` and verifies redaction is applied for + each of the four bypass paths (SLM, summarizer, hook, router). + Mock data: a request whose user message contains an + `ANTHROPIC_API_KEY=sk-ant-...` literal; assert the inner provider + sees the redacted form. +- [ ] Confirm `make test` is green across the security, slm, context, + hook, and router packages. + +### W1-5 — Docs + +- [ ] Update `docs/essentials/decisions/` with a new ADR + (`004-safe-provider-boundary.md` or next index) capturing: + - why scanning moved to the provider boundary + - the FirewallRef late-binding pattern + - the explicit decision to keep engine-level scanning for one + release +- [ ] Update `docs/essentials/INDEX.md` to reference the new ADR. + +--- + +## Exit criteria + +- Every provider arm registered in `router.Router` is a `SafeProvider`. + Verified by a startup-time assertion or test that iterates + `rtr.LookupArm` for each registered ID and type-asserts the + `Provider` field. +- A request whose user message contains a secret-shaped string + (`sk-ant-...`) is redacted before reaching the inner provider for + all four bypass paths (SLM classifier, summarizer, prompt hook, + router stream). +- `make test` and `make lint` green. +- No change to public `provider.Provider` interface. + +--- + +## Out of scope (deferred to Wave 2 / Wave 3) + +- Forced-arm + incognito local-only collision + (`router.go:67-73`) — Wave 2. +- Unconditional `persist.New(sessionID)` in incognito — Wave 2. +- TUI `m.incognito` not initialized from `fw.Incognito().Active()` + — Wave 2. +- `saveQuality()` / `ReportOutcome()` gating on CLI flag instead of + firewall state — Wave 2. +- PEM block regex completion — Wave 3. +- Optional strict high-entropy mode for non-local arms — Wave 3. +- Canonical `AllowedPaths` (Clean+Prefix → Abs+EvalSymlinks) — Wave 3. +- MCP `PathSensitiveTool` policy hook — Wave 3. +- `fs.grep` per-file symlink resolution — Wave 3. +- PostToolUse hook ordering vs. redaction — open question, see + "Tool-result hook ordering" below. + +### Tool-result hook ordering — open question + +The audit's finding #4 also covers PostToolUse hooks seeing raw tool +output before `ScanToolResult` runs. Wave 1 doesn't fix this because: + +- The audit's preferred fix ("redact, then fire hook") would change + the contract for command-type hooks (e.g. an audit-logger hook that + wants to record raw output). Splitting hooks into + `PostToolUseRawLocalOnly` vs. `PostToolUseRedactedForLLM` is a + design decision, not a refactor. +- Mitigation already exists: tool output written to the engine's + message history *is* redacted; the hook leak is only for prompt + hooks that re-inject hook-supplied content into an LLM. Today, no + shipped hook does this with raw tool output. + +Tracked separately. If/when a prompt-type PostToolUse hook ships, the +split-contract design must land first. + +--- + +## Effort estimate + +- W1-1: ~80 LOC + ~120 LOC tests. +- W1-2: ~20 LOC. +- W1-3: ~50 LOC across 5 call sites. +- W1-4: ~150 LOC tests. +- W1-5: ~80 lines of ADR. + +Total: ~500 LOC including tests. One PR or two, at your call. + +--- + +## Changelog + +- 2026-05-19: Initial. Captures Wave 1 of the post-audit hardening plan. diff --git a/internal/security/safeprovider.go b/internal/security/safeprovider.go new file mode 100644 index 0000000..206eb35 --- /dev/null +++ b/internal/security/safeprovider.go @@ -0,0 +1,66 @@ +package security + +import ( + "context" + + "somegit.dev/Owlibou/gnoma/internal/provider" + "somegit.dev/Owlibou/gnoma/internal/stream" +) + +// SafeProvider wraps a provider.Provider with firewall redaction. +// +// On every Stream call it scans outgoing messages and the system prompt +// through the firewall referenced by fwRef before delegating to the +// inner provider. Name, Models, and DefaultModel pass through unchanged. +// +// Tool-result redaction stays in the engine because it needs per-tool +// context (origin tool name for logging) that isn't available at the +// provider boundary. +// +// SafeProvider is the single non-bypassable boundary between gnoma's +// internals and any LLM endpoint. Every provider.Provider registered +// with the router or handed to a non-engine consumer (SLM classifier, +// summarizer, hook streamer) should be wrapped. +type SafeProvider struct { + inner provider.Provider + fwRef *FirewallRef +} + +// WrapProvider returns a SafeProvider that delegates to inner and +// resolves its firewall through ref. A nil ref is permitted and +// disables scanning — callers who never install a firewall get +// pass-through behaviour rather than a panic. +func WrapProvider(inner provider.Provider, ref *FirewallRef) *SafeProvider { + return &SafeProvider{inner: inner, fwRef: ref} +} + +// Inner returns the wrapped provider. Useful for tests and for code +// that needs to reach through to provider-specific behaviour. +func (p *SafeProvider) Inner() provider.Provider { + return p.inner +} + +func (p *SafeProvider) Stream(ctx context.Context, req provider.Request) (stream.Stream, error) { + if p.fwRef != nil { + if fw := p.fwRef.Get(); fw != nil { + req.Messages = fw.ScanOutgoingMessages(req.Messages) + req.SystemPrompt = fw.ScanSystemPrompt(req.SystemPrompt) + } + } + return p.inner.Stream(ctx, req) +} + +func (p *SafeProvider) Name() string { + return p.inner.Name() +} + +func (p *SafeProvider) Models(ctx context.Context) ([]provider.ModelInfo, error) { + return p.inner.Models(ctx) +} + +func (p *SafeProvider) DefaultModel() string { + return p.inner.DefaultModel() +} + +// Compile-time assertion that *SafeProvider satisfies provider.Provider. +var _ provider.Provider = (*SafeProvider)(nil) diff --git a/internal/security/safeprovider_test.go b/internal/security/safeprovider_test.go new file mode 100644 index 0000000..aad697f --- /dev/null +++ b/internal/security/safeprovider_test.go @@ -0,0 +1,243 @@ +package security + +import ( + "context" + "fmt" + "strings" + "sync" + "testing" + + "somegit.dev/Owlibou/gnoma/internal/message" + "somegit.dev/Owlibou/gnoma/internal/provider" + "somegit.dev/Owlibou/gnoma/internal/stream" +) + +// --- FirewallRef --- + +func TestFirewallRef_GetBeforeSetReturnsNil(t *testing.T) { + ref := new(FirewallRef) + if fw := ref.Get(); fw != nil { + t.Errorf("Get() before Set() = %v, want nil", fw) + } +} + +func TestFirewallRef_GetAfterSetReturnsValue(t *testing.T) { + ref := new(FirewallRef) + fw := NewFirewall(FirewallConfig{ScanOutgoing: true}) + ref.Set(fw) + if got := ref.Get(); got != fw { + t.Errorf("Get() = %p, want %p", got, fw) + } +} + +func TestFirewallRef_SetOverwritesPrevious(t *testing.T) { + ref := new(FirewallRef) + fw1 := NewFirewall(FirewallConfig{ScanOutgoing: true}) + fw2 := NewFirewall(FirewallConfig{ScanOutgoing: true}) + ref.Set(fw1) + ref.Set(fw2) + if got := ref.Get(); got != fw2 { + t.Errorf("Get() = %p, want %p (second Set)", got, fw2) + } +} + +func TestFirewallRef_ConcurrentSetAndGetIsRaceSafe(t *testing.T) { + ref := new(FirewallRef) + fw := NewFirewall(FirewallConfig{ScanOutgoing: true}) + + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(2) + go func() { + defer wg.Done() + ref.Set(fw) + }() + go func() { + defer wg.Done() + _ = ref.Get() + }() + } + wg.Wait() + + if got := ref.Get(); got != fw { + t.Errorf("after concurrent ops Get() = %p, want %p", got, fw) + } +} + +// --- recordingProvider --- + +// recordingProvider captures the last Request it saw and lets tests +// assert what reached the provider boundary. +type recordingProvider struct { + name string + lastReq provider.Request + streamErr error +} + +func (p *recordingProvider) Name() string { return p.name } +func (p *recordingProvider) DefaultModel() string { return "rec-model" } +func (p *recordingProvider) Models(_ context.Context) ([]provider.ModelInfo, error) { + return []provider.ModelInfo{{ + ID: "rec-model", + Name: "rec-model", + Provider: p.name, + }}, nil +} +func (p *recordingProvider) Stream(_ context.Context, req provider.Request) (stream.Stream, error) { + p.lastReq = req + if p.streamErr != nil { + return nil, p.streamErr + } + return &noopStream{}, nil +} + +type noopStream struct{} + +func (s *noopStream) Next() bool { return false } +func (s *noopStream) Current() stream.Event { return stream.Event{} } +func (s *noopStream) Err() error { return nil } +func (s *noopStream) Close() error { return nil } + +// --- SafeProvider --- + +func TestSafeProvider_NilRefDelegatesWithoutScanning(t *testing.T) { + rec := &recordingProvider{name: "rec"} + sp := WrapProvider(rec, nil) + + const secret = "sk-ant-api03-abcdefghijklmnopqrstuvwxyz" + req := provider.Request{ + SystemPrompt: "system contains " + secret, + Messages: []message.Message{ + message.NewUserText("user contains " + secret), + }, + } + + if _, err := sp.Stream(context.Background(), req); err != nil { + t.Fatalf("Stream() err = %v", err) + } + + if !strings.Contains(rec.lastReq.SystemPrompt, secret) { + t.Errorf("nil ref scrubbed system prompt: %q", rec.lastReq.SystemPrompt) + } + if got := rec.lastReq.Messages[0].TextContent(); !strings.Contains(got, secret) { + t.Errorf("nil ref scrubbed user message: %q", got) + } +} + +func TestSafeProvider_EmptyRefDelegatesWithoutScanning(t *testing.T) { + // A *FirewallRef whose pointer is unset should behave like nil ref. + rec := &recordingProvider{name: "rec"} + sp := WrapProvider(rec, new(FirewallRef)) + + const secret = "sk-ant-api03-abcdefghijklmnopqrstuvwxyz" + req := provider.Request{ + Messages: []message.Message{message.NewUserText(secret)}, + } + + if _, err := sp.Stream(context.Background(), req); err != nil { + t.Fatalf("Stream() err = %v", err) + } + + if got := rec.lastReq.Messages[0].TextContent(); !strings.Contains(got, secret) { + t.Errorf("empty ref scrubbed message: %q", got) + } +} + +func TestSafeProvider_RedactsOutgoingMessages(t *testing.T) { + rec := &recordingProvider{name: "rec"} + ref := new(FirewallRef) + ref.Set(NewFirewall(FirewallConfig{ + ScanOutgoing: true, + EntropyThreshold: 4.5, + })) + sp := WrapProvider(rec, ref) + + const secret = "sk-ant-api03-abcdefghijklmnopqrstuvwxyz" + req := provider.Request{ + Messages: []message.Message{ + message.NewUserText("here is my key: " + secret), + }, + } + + if _, err := sp.Stream(context.Background(), req); err != nil { + t.Fatalf("Stream() err = %v", err) + } + + got := rec.lastReq.Messages[0].TextContent() + if strings.Contains(got, secret) { + t.Errorf("secret leaked to inner provider: %q", got) + } + if !strings.Contains(got, "[REDACTED]") { + t.Errorf("expected [REDACTED] marker, got %q", got) + } +} + +func TestSafeProvider_RedactsSystemPrompt(t *testing.T) { + rec := &recordingProvider{name: "rec"} + ref := new(FirewallRef) + ref.Set(NewFirewall(FirewallConfig{ + ScanOutgoing: true, + EntropyThreshold: 4.5, + })) + sp := WrapProvider(rec, ref) + + const secret = "sk-ant-api03-abcdefghijklmnopqrstuvwxyz" + req := provider.Request{ + SystemPrompt: "operator key " + secret, + } + + if _, err := sp.Stream(context.Background(), req); err != nil { + t.Fatalf("Stream() err = %v", err) + } + + if strings.Contains(rec.lastReq.SystemPrompt, secret) { + t.Errorf("secret leaked in system prompt: %q", rec.lastReq.SystemPrompt) + } + if !strings.Contains(rec.lastReq.SystemPrompt, "[REDACTED]") { + t.Errorf("expected [REDACTED] marker, got %q", rec.lastReq.SystemPrompt) + } +} + +func TestSafeProvider_PassesThroughStreamError(t *testing.T) { + sentinel := fmt.Errorf("provider exploded") + rec := &recordingProvider{name: "rec", streamErr: sentinel} + sp := WrapProvider(rec, nil) + + _, err := sp.Stream(context.Background(), provider.Request{}) + if err != sentinel { + t.Errorf("Stream() err = %v, want %v", err, sentinel) + } +} + +func TestSafeProvider_PassesThroughName(t *testing.T) { + rec := &recordingProvider{name: "anthropic"} + sp := WrapProvider(rec, nil) + if got := sp.Name(); got != "anthropic" { + t.Errorf("Name() = %q, want %q", got, "anthropic") + } +} + +func TestSafeProvider_PassesThroughDefaultModel(t *testing.T) { + rec := &recordingProvider{name: "rec"} + sp := WrapProvider(rec, nil) + if got := sp.DefaultModel(); got != "rec-model" { + t.Errorf("DefaultModel() = %q, want %q", got, "rec-model") + } +} + +func TestSafeProvider_PassesThroughModels(t *testing.T) { + rec := &recordingProvider{name: "rec"} + sp := WrapProvider(rec, nil) + models, err := sp.Models(context.Background()) + if err != nil { + t.Fatalf("Models() err = %v", err) + } + if len(models) != 1 || models[0].ID != "rec-model" { + t.Errorf("Models() = %+v, want one model rec-model", models) + } +} + +func TestSafeProvider_SatisfiesProviderInterface(t *testing.T) { + // Compile-time check that *SafeProvider implements provider.Provider. + var _ provider.Provider = (*SafeProvider)(nil) +} diff --git a/internal/security/saferef.go b/internal/security/saferef.go new file mode 100644 index 0000000..8336946 --- /dev/null +++ b/internal/security/saferef.go @@ -0,0 +1,28 @@ +package security + +import "sync/atomic" + +// FirewallRef is a late-binding holder for *Firewall. +// +// Construction order in gnoma builds provider arms before the firewall +// exists. SafeProvider takes a *FirewallRef at construction time, then +// resolves the current *Firewall on each call. This lets the wiring be +// installed before NewFirewall runs without any locking on the hot path. +// +// A nil *FirewallRef or a *FirewallRef whose pointer has not been Set +// is interpreted by SafeProvider as "no firewall installed yet" — +// requests pass through unmodified. +type FirewallRef struct { + p atomic.Pointer[Firewall] +} + +// Set installs fw as the active firewall. Safe for concurrent use. +func (r *FirewallRef) Set(fw *Firewall) { + r.p.Store(fw) +} + +// Get returns the currently installed firewall, or nil if none has been +// Set. Safe for concurrent use. +func (r *FirewallRef) Get() *Firewall { + return r.p.Load() +} -- 2.54.0 From c932f1e3077832376056ecaef70341ce794b32e9 Mon Sep 17 00:00:00 2001 From: vikingowl Date: Tue, 19 May 2026 22:33:24 +0200 Subject: [PATCH 2/3] feat(security): wire SafeProvider into all provider sites (W1-2/3/4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Construct security.FirewallRef early in main() and Set it immediately after security.NewFirewall returns. Wrap every provider that may be called outside engine.buildRequest(): - primary provider arm (limitedProvider) - discovered local models (RegisterDiscoveredModels factory) - CLI agent arms (subprocprov.New) - background-discovery factory (StartDiscoveryLoop) - SLM arm + classifier transport - summarizer (gnomactx.NewSummarizeStrategy) routerStreamer and hook PromptExecutor inherit redaction automatically once every router arm is wrapped — they dispatch through router.Stream → arm.Provider.Stream. engine.Config.Provider stays raw because the engine still scans inline at buildRequest(); per the Wave 1 plan, removing that scan is deferred one release as belt-and-suspenders. Integration tests in internal/security/integration_test.go verify the boundary end-to-end: a router arm wrapped with WrapProvider redacts an 'sk-ant-...' literal before the inner provider sees it, and the pre-Set / post-Set transition works as documented (pass-through until the FirewallRef has a Firewall installed). --- cmd/gnoma/main.go | 33 +++++-- internal/security/integration_test.go | 135 ++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 internal/security/integration_test.go diff --git a/cmd/gnoma/main.go b/cmd/gnoma/main.go index c35b24c..4412f4e 100644 --- a/cmd/gnoma/main.go +++ b/cmd/gnoma/main.go @@ -342,6 +342,12 @@ func main() { // sessions from cross-contaminating the resume list. sessStore := session.NewSessionStoreAt(profile.SessionDir(gnomacfg.ProjectRoot()), cfg.Session.MaxKeep, logger) + // FirewallRef holds the *Firewall via atomic.Pointer so it can be + // installed into SafeProvider wrappers before NewFirewall runs below + // (~line 509). Until Set, wrappers pass through unmodified — matching + // pre-firewall behaviour for early-init code paths. + fwRef := new(security.FirewallRef) + // Create router and register the provider as a single arm // (M4 foundation: one provider from CLI. Multi-provider routing comes with config.) rtr := router.New(router.Config{Logger: logger}) @@ -392,7 +398,7 @@ func main() { } } armID = router.NewArmID(*providerName, armModel) - armProvider := limitedProvider(prov, *providerName, armModel, cfg) + armProvider := security.WrapProvider(limitedProvider(prov, *providerName, armModel, cfg), fwRef) arm := &router.Arm{ ID: armID, Provider: armProvider, @@ -423,7 +429,7 @@ func main() { if err != nil { return nil } - return p + return security.WrapProvider(p, fwRef) }) if len(localModels) > 0 { logger.Debug("local models discovered", "count", len(localModels)) @@ -435,7 +441,7 @@ func main() { if _, exists := rtr.LookupArm(cliArmID); !exists { rtr.RegisterArm(&router.Arm{ ID: cliArmID, - Provider: subprocprov.New(agent), + Provider: security.WrapProvider(subprocprov.New(agent), fwRef), ModelName: agent.Name, IsCLIAgent: true, Capabilities: agent.Capabilities, @@ -481,7 +487,7 @@ func main() { if err != nil { return nil } - return p + return security.WrapProvider(p, fwRef) } router.StartDiscoveryLoop(discoveryCtx, rtr, logger, cfg.Provider.Endpoints["ollama"], @@ -512,6 +518,10 @@ func main() { EntropyThreshold: entropyThreshold, Logger: logger, }) + // Install into the ref so every SafeProvider wrapper sees scanning + // from this point on. Wrappers created before this Set call + // pass through; wrappers created after see the active firewall. + fwRef.Set(fw) // Wire custom scanner patterns from config for _, p := range cfg.Security.Patterns { action := security.ActionRedact @@ -699,8 +709,11 @@ func main() { logger.Debug("context window from arm capabilities", "arm", armID, "context_window", contextWindowSize) } - // Create context window with summarize strategy (falls back to truncation) - var compactStrategy gnomactx.Strategy = gnomactx.NewSummarizeStrategy(prov) + // Create context window with summarize strategy (falls back to truncation). + // The summarizer talks to the provider directly (no engine.buildRequest in + // between), so it needs the SafeProvider wrapper to inherit firewall + // scanning. The engine itself still scans inline at buildRequest(). + var compactStrategy gnomactx.Strategy = gnomactx.NewSummarizeStrategy(security.WrapProvider(prov, fwRef)) ctxWindow := gnomactx.NewWindow(gnomactx.WindowConfig{ MaxTokens: contextWindowSize, Strategy: compactStrategy, @@ -746,7 +759,11 @@ func main() { case boot == nil: fmt.Fprintln(os.Stderr, "SLM unavailable: no backend reachable — using heuristic classifier.") default: - lazy.set(slm.NewClassifier(boot.Provider, boot.Model, logger)) + // Wrap once — the SLM provider is used both as the classifier + // transport and as a router arm. Both paths route through the + // firewall after fwRef.Set fires above. + slmProvider := security.WrapProvider(boot.Provider, fwRef) + lazy.set(slm.NewClassifier(slmProvider, boot.Model, logger)) // ToolUse comes from the live probe of the actual model. For // completion-only models (e.g. TinyLlama), the SLM arm only // handles knowledge-only prompts where the trivial-prompt @@ -755,7 +772,7 @@ func main() { // by MaxComplexity=0.3. rtr.RegisterArm(&router.Arm{ ID: router.ArmID("slm/" + string(boot.Backend)), - Provider: boot.Provider, + Provider: slmProvider, ModelName: boot.Model, IsLocal: true, MaxComplexity: 0.3, diff --git a/internal/security/integration_test.go b/internal/security/integration_test.go new file mode 100644 index 0000000..fa44c34 --- /dev/null +++ b/internal/security/integration_test.go @@ -0,0 +1,135 @@ +package security_test + +import ( + "context" + "strings" + "testing" + + "somegit.dev/Owlibou/gnoma/internal/message" + "somegit.dev/Owlibou/gnoma/internal/provider" + "somegit.dev/Owlibou/gnoma/internal/router" + "somegit.dev/Owlibou/gnoma/internal/security" + "somegit.dev/Owlibou/gnoma/internal/stream" +) + +// External test package (security_test) so we exercise the public surface +// the way main.go does — WrapProvider in front of an arm, router.Stream +// dispatches through it, the inner provider sees redacted content. + +type capturingProvider struct { + name string + lastReq provider.Request +} + +func (p *capturingProvider) Name() string { return p.name } +func (p *capturingProvider) DefaultModel() string { return "cap-model" } +func (p *capturingProvider) Models(_ context.Context) ([]provider.ModelInfo, error) { + return []provider.ModelInfo{{ID: "cap-model", Name: "cap-model", Provider: p.name}}, nil +} +func (p *capturingProvider) Stream(_ context.Context, req provider.Request) (stream.Stream, error) { + p.lastReq = req + return &nopStream{}, nil +} + +type nopStream struct{} + +func (s *nopStream) Next() bool { return false } +func (s *nopStream) Current() stream.Event { return stream.Event{} } +func (s *nopStream) Err() error { return nil } +func (s *nopStream) Close() error { return nil } + +func TestRouterArmWrappedWithSafeProvider_RedactsBeforeDelivery(t *testing.T) { + cap := &capturingProvider{name: "cap"} + ref := new(security.FirewallRef) + ref.Set(security.NewFirewall(security.FirewallConfig{ + ScanOutgoing: true, + EntropyThreshold: 4.5, + })) + + rtr := router.New(router.Config{}) + rtr.RegisterArm(&router.Arm{ + ID: router.NewArmID("cap", "cap-model"), + Provider: security.WrapProvider(cap, ref), + ModelName: "cap-model", + IsLocal: true, + Capabilities: provider.Capabilities{ToolUse: false}, + }) + + const secret = "sk-ant-api03-abcdefghijklmnopqrstuvwxyz" + req := provider.Request{ + Messages: []message.Message{ + message.NewUserText("here is my key: " + secret), + }, + } + + s, decision, err := rtr.Stream(context.Background(), router.Task{Type: router.TaskReview}, req) + if err != nil { + t.Fatalf("router.Stream err = %v", err) + } + defer func() { _ = s.Close() }() + decision.Commit(0) + + got := cap.lastReq.Messages[0].TextContent() + if strings.Contains(got, secret) { + t.Errorf("secret reached inner provider via router: %q", got) + } + if !strings.Contains(got, "[REDACTED]") { + t.Errorf("expected [REDACTED] marker after router dispatch, got %q", got) + } +} + +func TestRouterArmWrappedBeforeFirewallSet_PassesThroughUntilSet(t *testing.T) { + // Mirrors the construction order in main.go: arm is wrapped with a + // FirewallRef whose pointer isn't installed yet. A Stream call in that + // state must pass through unmodified; once Set fires, subsequent calls + // are scanned. + cap := &capturingProvider{name: "cap"} + ref := new(security.FirewallRef) // not Set yet + + rtr := router.New(router.Config{}) + rtr.RegisterArm(&router.Arm{ + ID: router.NewArmID("cap", "cap-model"), + Provider: security.WrapProvider(cap, ref), + ModelName: "cap-model", + IsLocal: true, + Capabilities: provider.Capabilities{ToolUse: false}, + }) + + const secret = "sk-ant-api03-abcdefghijklmnopqrstuvwxyz" + req := provider.Request{ + Messages: []message.Message{message.NewUserText(secret)}, + } + + // First call — ref unset, must pass through. + s, decision, err := rtr.Stream(context.Background(), router.Task{Type: router.TaskReview}, req) + if err != nil { + t.Fatalf("router.Stream (pre-Set) err = %v", err) + } + _ = s.Close() + decision.Commit(0) + + if got := cap.lastReq.Messages[0].TextContent(); !strings.Contains(got, secret) { + t.Fatalf("pre-Set call was modified: %q", got) + } + + // Now install the firewall and call again. + ref.Set(security.NewFirewall(security.FirewallConfig{ + ScanOutgoing: true, + EntropyThreshold: 4.5, + })) + + s, decision, err = rtr.Stream(context.Background(), router.Task{Type: router.TaskReview}, req) + if err != nil { + t.Fatalf("router.Stream (post-Set) err = %v", err) + } + _ = s.Close() + decision.Commit(0) + + got := cap.lastReq.Messages[0].TextContent() + if strings.Contains(got, secret) { + t.Errorf("secret reached inner provider after Set: %q", got) + } + if !strings.Contains(got, "[REDACTED]") { + t.Errorf("expected [REDACTED] after Set, got %q", got) + } +} -- 2.54.0 From eb3f3d71343875dab086026c2d2b89ce98a33565 Mon Sep 17 00:00:00 2001 From: vikingowl Date: Tue, 19 May 2026 22:37:24 +0200 Subject: [PATCH 3/3] feat(security): wrap engine.Config.Provider + SetProvider doc (W1 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Advisor flagged that engine.Config.Provider stayed raw, so the safety property was 'every call goes through buildRequest' instead of the stronger 'every Stream call routes through a SafeProvider.' Wrap it even though buildRequest still scans inline — at worst this costs one extra idempotent scan pass; it removes the 'someone adds a fifth engine Stream site that skips buildRequest' failure mode. Engine.SetProvider gets a doc comment establishing the wrap contract for callers. No active callers today, but documenting it now prevents the future bypass. Confirmed elf engines inherit the wrap automatically: - elf.Manager.Spawn passes arm.Provider (already *SafeProvider after W1-3a) - elf.Manager.SpawnWithProvider has no callers — dead code path Added the Wave 1 plan to TODO.md under active plans. --- TODO.md | 7 +++++++ cmd/gnoma/main.go | 5 ++++- internal/engine/engine.go | 7 +++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/TODO.md b/TODO.md index 21f6599..887609c 100644 --- a/TODO.md +++ b/TODO.md @@ -2,6 +2,13 @@ Active plans, newest first: +- **[`docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md`](docs/superpowers/plans/2026-05-19-security-wave1-safeprovider.md)** + — post-audit hardening, Wave 1. Closes the four firewall-bypass + call sites (SLM classifier, summarizer, prompt hook, routerStreamer) + by introducing `security.SafeProvider` at the provider boundary. + **In progress on `feat/security-wave1-safeprovider`** — implementation + complete; ADR and merge pending. Waves 2 (incognito coherence) and + 3 (scanner + path hygiene) are scoped but not yet drafted. - **[`docs/superpowers/plans/2026-05-19-post-slm-unlock.md`](docs/superpowers/plans/2026-05-19-post-slm-unlock.md)** — outstanding work after the SLM unlock session. Phases A (two-stage tool routing), B (CLI agent binary override), C (user profiles), and diff --git a/cmd/gnoma/main.go b/cmd/gnoma/main.go index 4412f4e..f477652 100644 --- a/cmd/gnoma/main.go +++ b/cmd/gnoma/main.go @@ -803,7 +803,10 @@ func main() { // Create engine eng, err := engine.New(engine.Config{ - Provider: prov, + // Wrap even though the engine's own buildRequest scans inline — + // belt-and-suspenders so a future engine path that bypasses + // buildRequest still routes through the firewall. + Provider: security.WrapProvider(prov, fwRef), Router: rtr, Classifier: engineClassifier, Tools: reg, diff --git a/internal/engine/engine.go b/internal/engine/engine.go index eca316c..ed7bef3 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -265,6 +265,13 @@ func (e *Engine) Usage() message.Usage { } // SetProvider swaps the active provider (for dynamic switching). +// +// Callers must pass a provider that has already been wrapped with +// security.WrapProvider — the engine's buildRequest scans inline today, +// but the boundary contract is "every Stream call routes through a +// SafeProvider." Passing a raw provider here would silently open a +// firewall bypass for any engine path that calls Provider.Stream +// without going through buildRequest. func (e *Engine) SetProvider(p provider.Provider) { e.mu.Lock() e.cfg.Provider = p -- 2.54.0