From e6f4de5ddc7c0a972b9fce828242cffbf6c43765 Mon Sep 17 00:00:00 2001 From: BaaRai <130491490+BaaRai@users.noreply.github.com> Date: Tue, 3 Nov 2026 22:10:41 +0100 Subject: [PATCH] fix(security): scrub startup secrets before any child can spawn The scrub ran in main after server.New returned, but New resumes every non-terminal task before returning, and those loops start workers that run shells. Move the scrub into New, right after both environment readers, and start MCP discovery after it as well: both paths spawn child processes. New takes an option rather than scrubbing unconditionally. The test suite builds several servers in one process and still needs ARTEX_PG_DSN on later calls, so only the binary opts in. --- cmd/artex/main.go | 16 +--------------- cmd/artex/main_test.go | 33 ++------------------------------- server/env_scrub.go | 30 ++++++++++++++++++++++++++++++ server/env_scrub_test.go | 36 ++++++++++++++++++++++++++++++++++++ server/server.go | 17 +++++++++++++---- 5 files changed, 82 insertions(+), 50 deletions(-) create mode 100644 server/env_scrub.go create mode 100644 server/env_scrub_test.go diff --git a/cmd/artex/main.go b/cmd/artex/main.go index 46c5ab3..056e496 100644 --- a/cmd/artex/main.go +++ b/cmd/artex/main.go @@ -93,8 +93,7 @@ func run() int { skillDir = abs } log.Printf("[config] skill directory: %s", skillDir) - srv := server.New(ctx, mgr, skillDir, *dataDir, config.BaseDir()) - scrubSecretEnv() + srv := server.New(ctx, mgr, skillDir, *dataDir, config.BaseDir(), server.WithSecretScrub()) httpSrv := &http.Server{ Addr: *addr, Handler: srv.Handler(), @@ -118,19 +117,6 @@ func run() int { return 0 } -// secretEnv lists the credentials read from the environment only at startup: -// ARTEX_PG_DSN by server.NewManager, the API keys by agent.FromEnv in server.New. -var secretEnv = []string{"ARTEX_PG_DSN", "ANTHROPIC_API_KEY", "OPENAI_API_KEY"} - -// scrubSecretEnv removes secretEnv from the process environment once startup has -// read it. Child processes (agent shells, custom tools, MCP stdio servers) copy -// os.Environ() when they are spawned, so they no longer inherit these values. -func scrubSecretEnv() { - for _, k := range secretEnv { - _ = os.Unsetenv(k) - } -} - // shutdownContext deliberately does not derive from signalCtx. If it did, the // parent's plain context.Canceled could win the race before AbortShutdown was // attached to the child, losing the diagnostic cause in every running Agent. diff --git a/cmd/artex/main_test.go b/cmd/artex/main_test.go index b89e44d..5e63227 100644 --- a/cmd/artex/main_test.go +++ b/cmd/artex/main_test.go @@ -2,42 +2,13 @@ package main import ( "context" - "os/exec" - "strings" "testing" "github.com/Autumn-27/artex/agent" ) -// A child process spawned after startup must not see the startup secrets, while -// other variables still pass through. -func TestScrubSecretEnvHidesSecretsFromChildren(t *testing.T) { - sh, err := exec.LookPath("sh") - if err != nil { - t.Skip("no sh in PATH") - } - for _, k := range secretEnv { - t.Setenv(k, "leak-"+k) - } - t.Setenv("ARTEX_SCRUB_KEEP", "kept") - - scrubSecretEnv() - - out, err := exec.Command(sh, "-c", "env").Output() - if err != nil { - t.Fatal(err) - } - env := "\n" + string(out) - for _, k := range secretEnv { - if strings.Contains(env, "\n"+k+"=") { - t.Errorf("%s still reaches child processes", k) - } - } - if !strings.Contains(env, "\nARTEX_SCRUB_KEEP=kept\n") { - t.Errorf("non-secret variable did not reach the child process") - } -} - +// The shutdown cause must survive the parent context being cancelled first; +// losing it would drop the diagnostic on every graceful stop. func TestShutdownContextPreservesNamedCause(t *testing.T) { signalCtx, signalCancel := context.WithCancel(context.Background()) ctx, shutdown := shutdownContext(signalCtx) diff --git a/server/env_scrub.go b/server/env_scrub.go new file mode 100644 index 0000000..e3c695e --- /dev/null +++ b/server/env_scrub.go @@ -0,0 +1,30 @@ +package server + +import "os" + +// secretEnv lists the credentials read from the environment only at startup: +// ARTEX_PG_DSN by NewManager, the API keys by agent.FromEnv inside New. +var secretEnv = []string{"ARTEX_PG_DSN", "ANTHROPIC_API_KEY", "OPENAI_API_KEY"} + +// ScrubSecretEnv removes secretEnv from the process environment once startup has +// read it. Child processes (agent shells, custom tools, MCP stdio servers) copy +// os.Environ() when they are spawned, so they no longer inherit these values. +// +// It runs inside New, after both readers and before the MCP discovery goroutine +// and the task resume loop: both can spawn children, and resuming a task starts +// workers that run shells. Scrubbing in main after New returned left that window +// open. +func ScrubSecretEnv() { + for _, k := range secretEnv { + _ = os.Unsetenv(k) + } +} + +// Option configures New. Options keep the test call sites unchanged: only the +// binary opts into scrubbing, because tests create several servers in one +// process and still need ARTEX_PG_DSN on later calls. +type Option func(*Server) + +// WithSecretScrub makes New drop the startup secrets after reading them, before +// the MCP discovery goroutine and the task resume loop can spawn children. +func WithSecretScrub() Option { return func(s *Server) { s.scrubSecrets = true } } diff --git a/server/env_scrub_test.go b/server/env_scrub_test.go new file mode 100644 index 0000000..a60b6b0 --- /dev/null +++ b/server/env_scrub_test.go @@ -0,0 +1,36 @@ +package server + +import ( + "os/exec" + "strings" + "testing" +) + +// A child process spawned after startup must not see the startup secrets, while +// other variables still pass through. +func TestScrubSecretEnvHidesSecretsFromChildren(t *testing.T) { + sh, err := exec.LookPath("sh") + if err != nil { + t.Skip("no sh in PATH") + } + for _, k := range secretEnv { + t.Setenv(k, "leak-"+k) + } + t.Setenv("ARTEX_SCRUB_KEEP", "kept") + + ScrubSecretEnv() + + out, err := exec.Command(sh, "-c", "env").Output() + if err != nil { + t.Fatal(err) + } + env := "\n" + string(out) + for _, k := range secretEnv { + if strings.Contains(env, "\n"+k+"=") { + t.Errorf("%s still reaches child processes", k) + } + } + if !strings.Contains(env, "\nARTEX_SCRUB_KEEP=kept\n") { + t.Errorf("non-secret variable did not reach the child process") + } +} diff --git a/server/server.go b/server/server.go index 1b898ab..a64b1da 100644 --- a/server/server.go +++ b/server/server.go @@ -46,6 +46,8 @@ type Server struct { skillDir string // root directory for skill subdirectories jwtKey []byte // HS256 signing key loaded from / generated into dataDir/jwt.key + scrubSecrets bool // drop ARTEX_PG_DSN and the API keys from the environment once read (binary only) + // concMu serializes concurrency-cap decisions (admission + reconcile) so a // scheduler tick and an HTTP settings change / task creation can't both count // free slots off the same snapshot and over-promote past the limit. @@ -140,7 +142,7 @@ type triggeredRun struct { mergeable bool // true for finding/goal event triggers (merge by taskID) } -func New(ctx context.Context, m *Manager, skillDir string, dataDir string, keyDir string) *Server { +func New(ctx context.Context, m *Manager, skillDir string, dataDir string, keyDir string, opts ...Option) *Server { key, err := loadOrCreateJWTKey(keyDir, dataDir) if err != nil { log.Fatalf("[auth] JWT key: %v", err) @@ -232,9 +234,6 @@ func New(ctx context.Context, m *Manager, skillDir string, dataDir string, keyDi // Vulnerability IM push delivery engine. Sits beside the Scheduler but independent: push latency demands (3s) // differ from the triggers' business cadence, and neither's failures drag the other down — a stuck push must not affect agent triggers. go newNotifier(s).Run(s.ctx) - // Fill the tool cache for any enabled MCP that has none yet (notably the - // seeded browser MCP on first run). Async so it never blocks startup. - go s.discoverEmptyMCPsOnStartup() logSink.SetDB(ctx, m.pg) // restore last 100 log rows and enable async persistence } // precedence: persisted DB config > env. @@ -253,6 +252,16 @@ func New(ctx context.Context, m *Manager, skillDir string, dataDir string, keyDi } else { log.Printf("[engine] no LLM provider configured — engine idle until set via /api/llm or env") } + // Both env readers have now run (NewManager for the DSN, FromEnv above for the + // keys). Drop the secrets before anything below can spawn a child process: + // restoring runtimes resumes workers that run shells, and MCP discovery starts + // stdio servers. + if s.scrubSecrets { + ScrubSecretEnv() + } + // Fill the tool cache for any enabled MCP that has none yet (notably the + // seeded browser MCP on first run). Async so it never blocks startup. + go s.discoverEmptyMCPsOnStartup() s.restoreTaskRuntimes() go s.reconcileConcurrency() s.startTaskArchiveWorker()