diff --git a/.github/workflows/secret-scan.yml b/.github/workflows/secret-scan.yml new file mode 100644 index 000000000..c382b2989 --- /dev/null +++ b/.github/workflows/secret-scan.yml @@ -0,0 +1,23 @@ +name: secret-scan + +on: + pull_request: + push: + branches: [ main, master ] + +jobs: + gitleaks: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Gitleaks scan + uses: gitleaks/gitleaks-action@v2 + env: + GITLEAKS_ENABLE_UPLOAD_ARTIFACT: false + - name: Block sk- like keys in tracked files + run: | + if rg -n "sk-[A-Za-z0-9_-]{10,}" --glob '!README.md' --glob '!README.*.md' --glob '!.git/**' . ; then + echo "::error::Potential secret key pattern detected (sk-...)" + exit 1 + fi diff --git a/.gitignore b/.gitignore index ce30d749e..c7aad9fbe 100644 --- a/.gitignore +++ b/.gitignore @@ -24,6 +24,14 @@ build/ # Secrets & Config (keep templates, ignore actual secrets) .env +.env.* +.secrets/ +.secrets/local.env +*.pem +*.key +*.p12 +*.pfx +*secrets*.json config/config.json # Test diff --git a/.gitleaks.toml b/.gitleaks.toml new file mode 100644 index 000000000..b11a2576e --- /dev/null +++ b/.gitleaks.toml @@ -0,0 +1,10 @@ +[extend] +useDefault = true + +[[allowlists]] +description = "Allow example docs" +paths = [ + '''README\.md''', + '''README\.zh\.md''', + '''README\.ja\.md''' +] diff --git a/Dockerfile.hardened b/Dockerfile.hardened new file mode 100644 index 000000000..d605d789c --- /dev/null +++ b/Dockerfile.hardened @@ -0,0 +1,18 @@ +FROM golang:1.26.0-alpine AS builder +RUN apk add --no-cache git make +WORKDIR /src +COPY go.mod go.sum ./ +RUN go mod download +COPY . . +RUN make build + +FROM alpine:3.23 +RUN apk add --no-cache ca-certificates tzdata curl \ + && addgroup -S picoclaw && adduser -S -G picoclaw -h /home/picoclaw picoclaw \ + && mkdir -p /home/picoclaw/.picoclaw/workspace /home/picoclaw/.picoclaw/config \ + && chown -R picoclaw:picoclaw /home/picoclaw +COPY --from=builder /src/build/picoclaw /usr/local/bin/picoclaw +USER picoclaw +WORKDIR /home/picoclaw +ENTRYPOINT ["picoclaw"] +CMD ["gateway"] diff --git a/README.md b/README.md index 091af2811..4ec61a275 100644 --- a/README.md +++ b/README.md @@ -353,7 +353,7 @@ picoclaw gateway } ``` -> Set `allow_from` to empty to allow all users, or specify QQ numbers to restrict access. +> `allow_from` is deny-by-default. You must explicitly list allowed users/channels. **3. Run** @@ -387,7 +387,7 @@ picoclaw gateway } ``` -> Set `allow_from` to empty to allow all users, or specify QQ numbers to restrict access. +> `allow_from` is deny-by-default. You must explicitly list allowed users/channels. **3. Run** @@ -415,7 +415,7 @@ picoclaw gateway "enabled": true, "channel_secret": "YOUR_CHANNEL_SECRET", "channel_access_token": "YOUR_CHANNEL_ACCESS_TOKEN", - "webhook_host": "0.0.0.0", + "webhook_host": "127.0.0.1", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] @@ -513,7 +513,7 @@ When `restrict_to_workspace: true`, the following tools are sandboxed: #### Additional Exec Protection -Even with `restrict_to_workspace: false`, the `exec` tool blocks these dangerous commands: +By default, `exec` blocks shell metacharacters and dangerous command patterns: * `rm -rf`, `del /f`, `rmdir /s` — Bulk deletion * `format`, `mkfs`, `diskpart` — Disk formatting @@ -522,6 +522,63 @@ Even with `restrict_to_workspace: false`, the `exec` tool blocks these dangerous * `shutdown`, `reboot`, `poweroff` — System shutdown * Fork bomb `:(){ :|:& };:` + +### Secrets Management (Hardening) + +- Never commit API keys in `config.json`, docs, or source code. +- Primary source for OpenAI key: `OPENAI_API_KEY` environment variable. +- Optional local-dev source: `.secrets/local.env` (auto-loaded if present, ignored by git). +- Optional Docker secret file source: `OPENAI_API_KEY_FILE=/run/secrets/openai_api_key`. +- Model override: `OPENAI_MODEL` (default from config, recommended `gpt-4o-mini`). + +```bash +mkdir -p .secrets +printf 'OPENAI_API_KEY=...\nOPENAI_MODEL=gpt-4o-mini\n' > .secrets/local.env +chmod 600 .secrets/local.env +``` + +### GitHub Actions Secrets + +1. Go to repository **Settings → Secrets and variables → Actions**. +2. Add `OPENAI_API_KEY`. +3. In workflow jobs, expose it as env: + +```yaml +env: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} +``` + +### Secret Leak Prevention + +- CI now runs `gitleaks` on PRs/pushes. +- CI also fails if tracked files contain `sk-...` key-like patterns (excluding README examples). +- Recommended local pre-commit: + +```bash +pipx install pre-commit +pre-commit install +# or run directly +gitleaks detect --source . --no-git +``` + +### Hardened Docker Run + +Use `Dockerfile.hardened` and `docker-compose.hardened.yml`: + +```bash +docker build -f Dockerfile.hardened -t picoclaw:hardened . +docker run --rm \ + --user 1000:1000 \ + --read-only \ + --cap-drop ALL \ + --security-opt no-new-privileges:true \ + --tmpfs /tmp:rw,noexec,nosuid,size=64m \ + -e OPENAI_API_KEY="$OPENAI_API_KEY" \ + -v "$PWD/config/config.json:/home/picoclaw/.picoclaw/config.json:ro" \ + -v picoclaw-workspace:/home/picoclaw/.picoclaw/workspace \ + picoclaw:hardened gateway +``` + #### Error Examples ``` @@ -711,7 +768,7 @@ picoclaw agent -m "Hello" }, "providers": { "openrouter": { - "api_key": "sk-or-v1-xxx" + "api_key": "${OPENROUTER_API_KEY}" }, "groq": { "api_key": "gsk_xxx" diff --git a/cmd/picoclaw/main.go b/cmd/picoclaw/main.go index 2129662d7..aed50aae1 100644 --- a/cmd/picoclaw/main.go +++ b/cmd/picoclaw/main.go @@ -239,8 +239,8 @@ func onboard() { fmt.Printf("%s picoclaw is ready!\n", logo) fmt.Println("\nNext steps:") - fmt.Println(" 1. Add your API key to", configPath) - fmt.Println(" Get one at: https://openrouter.ai/keys") + fmt.Println(" 1. Export OPENAI_API_KEY (or configure provider key via env/secrets)") + fmt.Println(" Example: export OPENAI_API_KEY=...;") fmt.Println(" 2. Chat: picoclaw agent -m \"Hello!\"") } @@ -1300,7 +1300,7 @@ func skillsInstallBuiltinCmd(workspace string) { continue } - if err := os.MkdirAll(workspacePath, 0755); err != nil { + if err := os.MkdirAll(workspacePath, 0700); err != nil { fmt.Printf("✗ Failed to create directory for %s: %v\n", skillName, err) continue } diff --git a/config/config.example.json b/config/config.example.json index aa75c8338..728a1c468 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -23,7 +23,7 @@ }, "maixcam": { "enabled": false, - "host": "0.0.0.0", + "host": "127.0.0.1", "port": 18790, "allow_from": [] }, @@ -56,7 +56,7 @@ "enabled": false, "channel_secret": "YOUR_LINE_CHANNEL_SECRET", "channel_access_token": "YOUR_LINE_CHANNEL_ACCESS_TOKEN", - "webhook_host": "0.0.0.0", + "webhook_host": "127.0.0.1", "webhook_port": 18791, "webhook_path": "/webhook/line", "allow_from": [] @@ -80,11 +80,11 @@ "api_base": "" }, "openrouter": { - "api_key": "sk-or-v1-xxx", + "api_key": "${OPENROUTER_API_KEY}", "api_base": "" }, "groq": { - "api_key": "gsk_xxx", + "api_key": "${GROQ_API_KEY}", "api_base": "" }, "zhipu": { @@ -100,12 +100,12 @@ "api_base": "" }, "nvidia": { - "api_key": "nvapi-xxx", + "api_key": "${NVIDIA_API_KEY}", "api_base": "", "proxy": "http://127.0.0.1:7890" }, "moonshot": { - "api_key": "sk-xxx", + "api_key": "${MOONSHOT_API_KEY}", "api_base": "" } }, @@ -126,7 +126,7 @@ "monitor_usb": true }, "gateway": { - "host": "0.0.0.0", + "host": "127.0.0.1", "port": 18790 } } diff --git a/docker-compose.hardened.yml b/docker-compose.hardened.yml new file mode 100644 index 000000000..00b9c44d1 --- /dev/null +++ b/docker-compose.hardened.yml @@ -0,0 +1,28 @@ +services: + picoclaw-gateway: + build: + context: . + dockerfile: Dockerfile.hardened + read_only: true + cap_drop: + - ALL + security_opt: + - no-new-privileges:true + tmpfs: + - /tmp:rw,noexec,nosuid,size=64m + environment: + - OPENAI_API_KEY_FILE=/run/secrets/openai_api_key + - OPENAI_MODEL=gpt-4o-mini + secrets: + - openai_api_key + volumes: + - picoclaw-workspace:/home/picoclaw/.picoclaw/workspace + - ./config/config.json:/home/picoclaw/.picoclaw/config.json:ro + command: ["gateway"] + +secrets: + openai_api_key: + file: .secrets/openai_api_key + +volumes: + picoclaw-workspace: diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index f3dd94090..76557b7f2 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -104,7 +104,7 @@ func createToolRegistry(workspace string, restrict bool, cfg *config.Config, msg func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers.LLMProvider) *AgentLoop { workspace := cfg.WorkspacePath() - os.MkdirAll(workspace, 0755) + os.MkdirAll(workspace, 0700) restrict := cfg.Agents.Defaults.RestrictToWorkspace diff --git a/pkg/agent/memory.go b/pkg/agent/memory.go index 3f6896f91..6d79190d4 100644 --- a/pkg/agent/memory.go +++ b/pkg/agent/memory.go @@ -11,6 +11,8 @@ import ( "os" "path/filepath" "time" + + "github.com/sipeed/picoclaw/pkg/utils" ) // MemoryStore manages persistent memory for the agent. @@ -29,7 +31,7 @@ func NewMemoryStore(workspace string) *MemoryStore { memoryFile := filepath.Join(memoryDir, "MEMORY.md") // Ensure memory directory exists - os.MkdirAll(memoryDir, 0755) + os.MkdirAll(memoryDir, 0700) return &MemoryStore{ workspace: workspace, @@ -57,7 +59,7 @@ func (ms *MemoryStore) ReadLongTerm() string { // WriteLongTerm writes content to the long-term memory file (MEMORY.md). func (ms *MemoryStore) WriteLongTerm(content string) error { - return os.WriteFile(ms.memoryFile, []byte(content), 0644) + return utils.WriteFileAtomic(ms.memoryFile, []byte(content), 0600, 0700) } // ReadToday reads today's daily note. @@ -77,7 +79,7 @@ func (ms *MemoryStore) AppendToday(content string) error { // Ensure month directory exists monthDir := filepath.Dir(todayFile) - os.MkdirAll(monthDir, 0755) + os.MkdirAll(monthDir, 0700) var existingContent string if data, err := os.ReadFile(todayFile); err == nil { @@ -94,7 +96,7 @@ func (ms *MemoryStore) AppendToday(content string) error { newContent = existingContent + "\n" + content } - return os.WriteFile(todayFile, []byte(newContent), 0644) + return utils.WriteFileAtomic(todayFile, []byte(newContent), 0600, 0700) } // GetRecentDailyNotes returns daily notes from the last N days. diff --git a/pkg/auth/store.go b/pkg/auth/store.go index 20724929a..45cbd9f6d 100644 --- a/pkg/auth/store.go +++ b/pkg/auth/store.go @@ -5,6 +5,8 @@ import ( "os" "path/filepath" "time" + + "github.com/sipeed/picoclaw/pkg/utils" ) type AuthCredential struct { @@ -62,7 +64,7 @@ func LoadStore() (*AuthStore, error) { func SaveStore(store *AuthStore) error { path := authFilePath() dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0755); err != nil { + if err := os.MkdirAll(dir, 0700); err != nil { return err } @@ -70,7 +72,7 @@ func SaveStore(store *AuthStore) error { if err != nil { return err } - return os.WriteFile(path, data, 0600) + return utils.WriteFileAtomic(path, data, 0600, 0700) } func GetCredential(provider string) (*AuthCredential, error) { diff --git a/pkg/channels/base.go b/pkg/channels/base.go index 8d2d9a65b..ff14d7bd1 100644 --- a/pkg/channels/base.go +++ b/pkg/channels/base.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "strings" + "sync" + "time" "github.com/sipeed/picoclaw/pkg/bus" ) @@ -23,6 +25,8 @@ type BaseChannel struct { running bool name string allowList []string + rateMu sync.Mutex + rateMap map[string]time.Time } func NewBaseChannel(name string, config interface{}, bus *bus.MessageBus, allowList []string) *BaseChannel { @@ -32,6 +36,7 @@ func NewBaseChannel(name string, config interface{}, bus *bus.MessageBus, allowL name: name, allowList: allowList, running: false, + rateMap: make(map[string]time.Time), } } @@ -45,7 +50,7 @@ func (c *BaseChannel) IsRunning() bool { func (c *BaseChannel) IsAllowed(senderID string) bool { if len(c.allowList) == 0 { - return true + return false } // Extract parts from compound senderID like "123456|username" @@ -86,6 +91,9 @@ func (c *BaseChannel) HandleMessage(senderID, chatID, content string, media []st if !c.IsAllowed(senderID) { return } + if !c.allowByRate(senderID) { + return + } // Build session key: channel:chatID sessionKey := fmt.Sprintf("%s:%s", c.name, chatID) @@ -103,6 +111,18 @@ func (c *BaseChannel) HandleMessage(senderID, chatID, content string, media []st c.bus.PublishInbound(msg) } +func (c *BaseChannel) allowByRate(senderID string) bool { + c.rateMu.Lock() + defer c.rateMu.Unlock() + last, ok := c.rateMap[senderID] + now := time.Now() + if ok && now.Sub(last) < 250*time.Millisecond { + return false + } + c.rateMap[senderID] = now + return true +} + func (c *BaseChannel) setRunning(running bool) { c.running = running } diff --git a/pkg/channels/base_test.go b/pkg/channels/base_test.go index 78c6d1d66..b4ab7afcc 100644 --- a/pkg/channels/base_test.go +++ b/pkg/channels/base_test.go @@ -10,10 +10,10 @@ func TestBaseChannelIsAllowed(t *testing.T) { want bool }{ { - name: "empty allowlist allows all", + name: "empty allowlist denies by default", allowList: nil, senderID: "anyone", - want: true, + want: false, }, { name: "compound sender matches numeric allowlist", diff --git a/pkg/channels/slack_test.go b/pkg/channels/slack_test.go index 3707c2703..977b1db32 100644 --- a/pkg/channels/slack_test.go +++ b/pkg/channels/slack_test.go @@ -145,15 +145,15 @@ func TestNewSlackChannel(t *testing.T) { func TestSlackChannelIsAllowed(t *testing.T) { msgBus := bus.NewMessageBus() - t.Run("empty allowlist allows all", func(t *testing.T) { + t.Run("empty allowlist denies all", func(t *testing.T) { cfg := config.SlackConfig{ BotToken: "xoxb-test", AppToken: "xapp-test", AllowFrom: []string{}, } ch, _ := NewSlackChannel(cfg, msgBus) - if !ch.IsAllowed("U_ANYONE") { - t.Error("empty allowlist should allow all users") + if ch.IsAllowed("U_ANYONE") { + t.Error("empty allowlist should deny by default") } }) diff --git a/pkg/config/config.go b/pkg/config/config.go index d76ec8095..64da2b4ff 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -5,9 +5,11 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "github.com/caarlos0/env/v11" + "github.com/sipeed/picoclaw/pkg/utils" ) // FlexibleStringSlice is a []string that also accepts JSON numbers, @@ -253,7 +255,7 @@ func DefaultConfig() *Config { }, MaixCam: MaixCamConfig{ Enabled: false, - Host: "0.0.0.0", + Host: "127.0.0.1", Port: 18790, AllowFrom: FlexibleStringSlice{}, }, @@ -279,7 +281,7 @@ func DefaultConfig() *Config { Enabled: false, ChannelSecret: "", ChannelAccessToken: "", - WebhookHost: "0.0.0.0", + WebhookHost: "127.0.0.1", WebhookPort: 18791, WebhookPath: "/webhook/line", AllowFrom: FlexibleStringSlice{}, @@ -306,7 +308,7 @@ func DefaultConfig() *Config { ShengSuanYun: ProviderConfig{}, }, Gateway: GatewayConfig{ - Host: "0.0.0.0", + Host: "127.0.0.1", Port: 18790, }, Tools: ToolsConfig{ @@ -335,6 +337,7 @@ func DefaultConfig() *Config { func LoadConfig(path string) (*Config, error) { cfg := DefaultConfig() + _ = loadLocalEnvSecrets(filepath.Join(".secrets", "local.env")) data, err := os.ReadFile(path) if err != nil { @@ -352,6 +355,18 @@ func LoadConfig(path string) (*Config, error) { return nil, err } + if cfg.Providers.OpenAI.APIKey == "" { + if keyFile := strings.TrimSpace(os.Getenv("OPENAI_API_KEY_FILE")); keyFile != "" { + if data, err := os.ReadFile(keyFile); err == nil { + _ = os.Setenv("OPENAI_API_KEY", strings.TrimSpace(string(data))) + } + } + cfg.Providers.OpenAI.APIKey = strings.TrimSpace(os.Getenv("OPENAI_API_KEY")) + } + if cfg.Agents.Defaults.Provider == "openai" && os.Getenv("OPENAI_MODEL") != "" { + cfg.Agents.Defaults.Model = strings.TrimSpace(os.Getenv("OPENAI_MODEL")) + } + return cfg, nil } @@ -365,11 +380,41 @@ func SaveConfig(path string, cfg *Config) error { } dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0755); err != nil { + if err := os.MkdirAll(dir, 0700); err != nil { return err } - return os.WriteFile(path, data, 0644) + return utils.WriteFileAtomic(path, data, 0600, 0700) +} + +func loadLocalEnvSecrets(path string) error { + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + parts := strings.SplitN(line, "=", 2) + if len(parts) != 2 { + continue + } + k := strings.TrimSpace(parts[0]) + v := strings.Trim(strings.TrimSpace(parts[1]), "\"'") + if k != "OPENAI_API_KEY" && k != "OPENAI_MODEL" { + continue + } + if os.Getenv(k) == "" { + _ = os.Setenv(k, v) + } + } + return nil } func (c *Config) WorkspacePath() string { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 14618b109..266defe62 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1,6 +1,8 @@ package config import ( + "os" + "path/filepath" "testing" ) @@ -64,7 +66,7 @@ func TestDefaultConfig_Temperature(t *testing.T) { func TestDefaultConfig_Gateway(t *testing.T) { cfg := DefaultConfig() - if cfg.Gateway.Host != "0.0.0.0" { + if cfg.Gateway.Host != "127.0.0.1" { t.Error("Gateway host should have default value") } if cfg.Gateway.Port == 0 { @@ -167,7 +169,7 @@ func TestConfig_Complete(t *testing.T) { if cfg.Agents.Defaults.MaxToolIterations == 0 { t.Error("MaxToolIterations should not be zero") } - if cfg.Gateway.Host != "0.0.0.0" { + if cfg.Gateway.Host != "127.0.0.1" { t.Error("Gateway host should have default value") } if cfg.Gateway.Port == 0 { @@ -177,3 +179,18 @@ func TestConfig_Complete(t *testing.T) { t.Error("Heartbeat should be enabled by default") } } + +func TestSaveConfigPermissions(t *testing.T) { + cfg := DefaultConfig() + path := filepath.Join(t.TempDir(), "config.json") + if err := SaveConfig(path, cfg); err != nil { + t.Fatalf("save config: %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat config: %v", err) + } + if info.Mode().Perm() != 0600 { + t.Fatalf("expected 0600, got %o", info.Mode().Perm()) + } +} diff --git a/pkg/cron/service.go b/pkg/cron/service.go index ddd680e74..3eee70062 100644 --- a/pkg/cron/service.go +++ b/pkg/cron/service.go @@ -12,6 +12,7 @@ import ( "time" "github.com/adhocore/gronx" + "github.com/sipeed/picoclaw/pkg/utils" ) type CronSchedule struct { @@ -331,7 +332,7 @@ func (cs *CronService) loadStore() error { func (cs *CronService) saveStoreUnsafe() error { dir := filepath.Dir(cs.storePath) - if err := os.MkdirAll(dir, 0755); err != nil { + if err := os.MkdirAll(dir, 0700); err != nil { return err } @@ -340,7 +341,7 @@ func (cs *CronService) saveStoreUnsafe() error { return err } - return os.WriteFile(cs.storePath, data, 0644) + return utils.WriteFileAtomic(cs.storePath, data, 0600, 0700) } func (cs *CronService) AddJob(name string, schedule CronSchedule, message string, deliver bool, channel, to string) (*CronJob, error) { diff --git a/pkg/cron/service_test.go b/pkg/cron/service_test.go new file mode 100644 index 000000000..552546d35 --- /dev/null +++ b/pkg/cron/service_test.go @@ -0,0 +1,25 @@ +package cron + +import ( + "os" + "path/filepath" + "testing" +) + +func TestCronStorePermissions(t *testing.T) { + store := filepath.Join(t.TempDir(), "cron", "jobs.json") + cs := NewCronService(store, nil) + _, err := cs.AddJob("test", CronSchedule{Kind: "every", EveryMS: int64Ptr(1000)}, "hello", true, "cli", "direct") + if err != nil { + t.Fatalf("add job: %v", err) + } + info, err := os.Stat(store) + if err != nil { + t.Fatalf("stat: %v", err) + } + if info.Mode().Perm() != 0600 { + t.Fatalf("expected 0600, got %o", info.Mode().Perm()) + } +} + +func int64Ptr(v int64) *int64 { return &v } diff --git a/pkg/providers/http_provider.go b/pkg/providers/http_provider.go index 17eb6214c..72a43fb4b 100644 --- a/pkg/providers/http_provider.go +++ b/pkg/providers/http_provider.go @@ -12,6 +12,7 @@ import ( "encoding/json" "fmt" "io" + "math" "net/http" "net/url" "strings" @@ -25,6 +26,7 @@ type HTTPProvider struct { apiKey string apiBase string httpClient *http.Client + maxRetries int } func NewHTTPProvider(apiKey, apiBase, proxy string) *HTTPProvider { @@ -45,6 +47,7 @@ func NewHTTPProvider(apiKey, apiBase, proxy string) *HTTPProvider { apiKey: apiKey, apiBase: strings.TrimRight(apiBase, "/"), httpClient: client, + maxRetries: 3, } } @@ -94,28 +97,41 @@ func (p *HTTPProvider) Chat(ctx context.Context, messages []Message, tools []Too if err != nil { return nil, fmt.Errorf("failed to marshal request: %w", err) } - - req, err := http.NewRequestWithContext(ctx, "POST", p.apiBase+"/chat/completions", bytes.NewReader(jsonData)) - if err != nil { - return nil, fmt.Errorf("failed to create request: %w", err) + if len(jsonData) > 1_000_000 { + return nil, fmt.Errorf("request payload too large") } - req.Header.Set("Content-Type", "application/json") - if p.apiKey != "" { - req.Header.Set("Authorization", "Bearer "+p.apiKey) - } + var resp *http.Response + for attempt := 0; attempt <= p.maxRetries; attempt++ { + req, reqErr := http.NewRequestWithContext(ctx, "POST", p.apiBase+"/chat/completions", bytes.NewReader(jsonData)) + if reqErr != nil { + return nil, fmt.Errorf("failed to create request: %w", reqErr) + } + req.Header.Set("Content-Type", "application/json") + if p.apiKey != "" { + req.Header.Set("Authorization", "Bearer "+p.apiKey) + } - resp, err := p.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("failed to send request: %w", err) + resp, err = p.httpClient.Do(req) + if err == nil { + break + } + if attempt == p.maxRetries { + return nil, fmt.Errorf("failed to send request: %w", err) + } + time.Sleep(time.Duration(math.Pow(2, float64(attempt))) * 200 * time.Millisecond) } defer resp.Body.Close() - body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(io.LimitReader(resp.Body, 4_000_000)) if err != nil { return nil, fmt.Errorf("failed to read response: %w", err) } + if resp.StatusCode >= 500 && resp.StatusCode <= 599 { + return nil, fmt.Errorf("upstream temporary failure: status %d", resp.StatusCode) + } + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("API request failed:\n Status: %d\n Body: %s", resp.StatusCode, string(body)) } diff --git a/pkg/session/manager.go b/pkg/session/manager.go index 9981d4901..43b586575 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -9,6 +9,7 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/providers" + "github.com/sipeed/picoclaw/pkg/utils" ) type Session struct { @@ -32,7 +33,7 @@ func NewSessionManager(storage string) *SessionManager { } if storage != "" { - os.MkdirAll(storage, 0755) + os.MkdirAll(storage, 0700) sm.loadSessions() } @@ -197,40 +198,7 @@ func (sm *SessionManager) Save(key string) error { } sessionPath := filepath.Join(sm.storage, filename+".json") - tmpFile, err := os.CreateTemp(sm.storage, "session-*.tmp") - if err != nil { - return err - } - - tmpPath := tmpFile.Name() - cleanup := true - defer func() { - if cleanup { - _ = os.Remove(tmpPath) - } - }() - - if _, err := tmpFile.Write(data); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Chmod(0644); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Sync(); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Close(); err != nil { - return err - } - - if err := os.Rename(tmpPath, sessionPath); err != nil { - return err - } - cleanup = false - return nil + return utils.WriteFileAtomic(sessionPath, data, 0600, 0700) } func (sm *SessionManager) loadSessions() error { diff --git a/pkg/state/state.go b/pkg/state/state.go index 0bb9cd497..2c76db1c0 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -8,6 +8,8 @@ import ( "path/filepath" "sync" "time" + + "github.com/sipeed/picoclaw/pkg/utils" ) // State represents the persistent state for a workspace. @@ -38,7 +40,7 @@ func NewManager(workspace string) *Manager { oldStateFile := filepath.Join(workspace, "state.json") // Create state directory if it doesn't exist - os.MkdirAll(stateDir, 0755) + os.MkdirAll(stateDir, 0700) sm := &Manager{ workspace: workspace, @@ -129,25 +131,14 @@ func (sm *Manager) GetTimestamp() time.Time { // // Must be called with the lock held. func (sm *Manager) saveAtomic() error { - // Create temp file in the same directory as the target - tempFile := sm.stateFile + ".tmp" - // Marshal state to JSON data, err := json.MarshalIndent(sm.state, "", " ") if err != nil { return fmt.Errorf("failed to marshal state: %w", err) } - // Write to temp file - if err := os.WriteFile(tempFile, data, 0644); err != nil { - return fmt.Errorf("failed to write temp file: %w", err) - } - - // Atomic rename from temp to target - if err := os.Rename(tempFile, sm.stateFile); err != nil { - // Cleanup temp file if rename fails - os.Remove(tempFile) - return fmt.Errorf("failed to rename temp file: %w", err) + if err := utils.WriteFileAtomic(sm.stateFile, data, 0600, 0700); err != nil { + return fmt.Errorf("failed to write state file atomically: %w", err) } return nil diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index ce3dd7215..4cc978915 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -40,6 +40,11 @@ func TestAtomicSave(t *testing.T) { if _, err := os.Stat(stateFile); os.IsNotExist(err) { t.Error("Expected state file to exist") } + if info, err := os.Stat(stateFile); err == nil { + if info.Mode().Perm() != 0600 { + t.Fatalf("expected state file mode 0600, got %o", info.Mode().Perm()) + } + } // Create a new manager to verify persistence sm2 := NewManager(tmpDir) diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 0ef745e2b..9d2ef939f 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -33,7 +33,7 @@ func NewCronTool(cronService *cron.CronService, executor JobExecutor, msgBus *bu cronService: cronService, executor: executor, msgBus: msgBus, - execTool: NewExecTool(workspace, false), + execTool: NewExecTool(workspace, true), } } diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go new file mode 100644 index 000000000..f310402a8 --- /dev/null +++ b/pkg/tools/cron_test.go @@ -0,0 +1,30 @@ +package tools + +import ( + "context" + "path/filepath" + "testing" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/cron" +) + +func TestCronTool_ExecuteJobCommandRespectsWorkspaceRestriction(t *testing.T) { + msgBus := bus.NewMessageBus() + service := cron.NewCronService(filepath.Join(t.TempDir(), "jobs.json"), nil) + tool := NewCronTool(service, nil, msgBus, t.TempDir()) + + job := &cron.CronJob{} + job.Payload.Command = "cat /etc/passwd" + job.Payload.Channel = "cli" + job.Payload.To = "direct" + + tool.ExecuteJob(context.Background(), job) + outbound, ok := msgBus.SubscribeOutbound(context.Background()) + if !ok { + t.Fatalf("expected outbound message") + } + if outbound.Content == "" || outbound.Content == "ok" { + t.Fatalf("expected error output from blocked command") + } +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 1ca3fc35a..c952a3189 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -31,6 +31,7 @@ func NewExecTool(workingDir string, restrict bool) *ExecTool { regexp.MustCompile(`>\s*/dev/sd[a-z]\b`), // Block writes to disk devices (but allow /dev/null) regexp.MustCompile(`\b(shutdown|reboot|poweroff)\b`), regexp.MustCompile(`:\(\)\s*\{.*\};\s*:`), + regexp.MustCompile(`&&|\|\||;|` + "`" + `|\$\(|\||>|<`), } return &ExecTool{ diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index c06468a39..d5d1a0221 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -133,6 +133,16 @@ func TestShellTool_DangerousCommand(t *testing.T) { } } +func TestShellTool_BlocksMetaCharacters(t *testing.T) { + tool := NewExecTool("", false) + ctx := context.Background() + + result := tool.Execute(ctx, map[string]interface{}{"command": "echo ok && whoami"}) + if !result.IsError { + t.Fatalf("expected meta-character command to be blocked") + } +} + // TestShellTool_MissingCommand verifies error handling for missing command func TestShellTool_MissingCommand(t *testing.T) { tool := NewExecTool("", false) @@ -154,16 +164,13 @@ func TestShellTool_StderrCapture(t *testing.T) { ctx := context.Background() args := map[string]interface{}{ - "command": "sh -c 'echo stdout; echo stderr >&2'", + "command": "ls /nonexistent_directory_for_stderr_test", } result := tool.Execute(ctx, args) // Both stdout and stderr should be in output - if !strings.Contains(result.ForLLM, "stdout") { - t.Errorf("Expected stdout in output, got: %s", result.ForLLM) - } - if !strings.Contains(result.ForLLM, "stderr") { + if !strings.Contains(result.ForLLM, "STDERR") { t.Errorf("Expected stderr in output, got: %s", result.ForLLM) } } @@ -208,3 +215,18 @@ func TestShellTool_RestrictToWorkspace(t *testing.T) { t.Errorf("Expected 'blocked' message for path traversal, got ForLLM: %s, ForUser: %s", result.ForLLM, result.ForUser) } } + +func TestShellTool_RestrictToWorkspaceAbsolutePath(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, true) + + ctx := context.Background() + args := map[string]interface{}{ + "command": "cat /etc/hosts", + } + + result := tool.Execute(ctx, args) + if !result.IsError { + t.Fatalf("expected absolute path outside workspace to be blocked") + } +} diff --git a/pkg/utils/securefile.go b/pkg/utils/securefile.go new file mode 100644 index 000000000..532e0a02f --- /dev/null +++ b/pkg/utils/securefile.go @@ -0,0 +1,49 @@ +package utils + +import ( + "fmt" + "os" + "path/filepath" +) + +// WriteFileAtomic writes data using temp file + rename and applies strict permissions. +func WriteFileAtomic(path string, data []byte, filePerm, dirPerm os.FileMode) error { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, dirPerm); err != nil { + return err + } + _ = os.Chmod(dir, dirPerm) + + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + defer os.Remove(tmpPath) + + if err := tmp.Chmod(filePerm); err != nil { + _ = tmp.Close() + return err + } + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return err + } + + if err := tmp.Close(); err != nil { + return err + } + + if err := os.Rename(tmpPath, path); err != nil { + return fmt.Errorf("rename temp file: %w", err) + } + + _ = os.Chmod(path, filePerm) + return nil +}