Fix #179 security hardening and #199 default model/CI

.github/workflows/ci.yml

@@ -0,0 +1,39 @@
+name: CI
+
+on:
+  push:
+    branches:
+      - main
+  pull_request:
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Setup Go
+        uses: actions/setup-go@v5
+        with:
+          go-version-file: go.mod
+          cache: true
+
+      - name: Check formatting
+        run: |
+          unformatted="$(gofmt -l .)"
+          if [ -n "$unformatted" ]; then
+            echo "The following files need gofmt:"
+            echo "$unformatted"
+            exit 1
+          fi
+
+      - name: Generate embedded workspace files
+        run: go generate ./cmd/picoclaw
+
+      - name: Run go vet
+        run: go vet ./...
+
+      - name: Run tests (race)
+        run: go test -race ./...

pkg/auth/store_test.go

@@ -3,10 +3,19 @@ package auth
 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 	"time"
 )
 
+func setTestHome(t *testing.T, dir string) {
+	t.Helper()
+	t.Setenv("HOME", dir)
+	t.Setenv("USERPROFILE", dir)
+	t.Setenv("HOMEDRIVE", "")
+	t.Setenv("HOMEPATH", "")
+}
+
 func TestAuthCredentialIsExpired(t *testing.T) {
 	tests := []struct {
 		name      string
@@ -52,9 +61,7 @@ func TestAuthCredentialNeedsRefresh(t *testing.T) {
 
 func TestStoreRoundtrip(t *testing.T) {
 	tmpDir := t.TempDir()
-	origHome := os.Getenv("HOME")
-	t.Setenv("HOME", tmpDir)
-	defer os.Setenv("HOME", origHome)
+	setTestHome(t, tmpDir)
 
 	cred := &AuthCredential{
 		AccessToken:  "test-access-token",
@@ -88,10 +95,12 @@ func TestStoreRoundtrip(t *testing.T) {
 }
 
 func TestStoreFilePermissions(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("file permission bits are not reliable on windows")
+	}
+
 	tmpDir := t.TempDir()
-	origHome := os.Getenv("HOME")
-	t.Setenv("HOME", tmpDir)
-	defer os.Setenv("HOME", origHome)
+	setTestHome(t, tmpDir)
 
 	cred := &AuthCredential{
 		AccessToken: "secret-token",
@@ -115,9 +124,7 @@ func TestStoreFilePermissions(t *testing.T) {
 
 func TestStoreMultiProvider(t *testing.T) {
 	tmpDir := t.TempDir()
-	origHome := os.Getenv("HOME")
-	t.Setenv("HOME", tmpDir)
-	defer os.Setenv("HOME", origHome)
+	setTestHome(t, tmpDir)
 
 	openaiCred := &AuthCredential{AccessToken: "openai-token", Provider: "openai", AuthMethod: "oauth"}
 	anthropicCred := &AuthCredential{AccessToken: "anthropic-token", Provider: "anthropic", AuthMethod: "token"}
@@ -148,9 +155,7 @@ func TestStoreMultiProvider(t *testing.T) {
 
 func TestDeleteCredential(t *testing.T) {
 	tmpDir := t.TempDir()
-	origHome := os.Getenv("HOME")
-	t.Setenv("HOME", tmpDir)
-	defer os.Setenv("HOME", origHome)
+	setTestHome(t, tmpDir)
 
 	cred := &AuthCredential{AccessToken: "to-delete", Provider: "openai", AuthMethod: "oauth"}
 	if err := SetCredential("openai", cred); err != nil {
@@ -172,9 +177,7 @@ func TestDeleteCredential(t *testing.T) {
 
 func TestLoadStoreEmpty(t *testing.T) {
 	tmpDir := t.TempDir()
-	origHome := os.Getenv("HOME")
-	t.Setenv("HOME", tmpDir)
-	defer os.Setenv("HOME", origHome)
+	setTestHome(t, tmpDir)
 
 	store, err := LoadStore()
 	if err != nil {

pkg/channels/slack.go

@@ -307,7 +307,6 @@ func (c *SlackChannel) handleAppMention(ev *slackevents.AppMentionEvent) {
 	if ev.User == c.botUserID {
 		return
 	}
-
 	if !c.IsAllowed(ev.User) {
 		logger.DebugCF("slack", "Mention rejected by allowlist", map[string]any{
 			"user_id": ev.User,
@@ -374,14 +373,15 @@ func (c *SlackChannel) handleSlashCommand(event socketmode.Event) {
 		c.socketClient.Ack(*event.Request)
 	}
 
-	if !c.IsAllowed(cmd.UserID) {
+	senderID := cmd.UserID
+	if !c.IsAllowed(senderID) {
 		logger.DebugCF("slack", "Slash command rejected by allowlist", map[string]any{
-			"user_id": cmd.UserID,
+			"user_id": senderID,
+			"command": cmd.Command,
 		})
 		return
 	}
 
-	senderID := cmd.UserID
 	channelID := cmd.ChannelID
 	chatID := channelID
 	content := cmd.Text

pkg/channels/slack_test.go

@@ -1,7 +1,13 @@
 package channels
 
 import (
+	"context"
 	"testing"
+	"time"
+
+	"github.com/slack-go/slack"
+	"github.com/slack-go/slack/slackevents"
+	"github.com/slack-go/slack/socketmode"
 
 	"github.com/sipeed/picoclaw/pkg/bus"
 	"github.com/sipeed/picoclaw/pkg/config"
@@ -172,3 +178,74 @@ func TestSlackChannelIsAllowed(t *testing.T) {
 		}
 	})
 }
+
+func TestSlackAppMentionRejectsUsersOutsideAllowlist(t *testing.T) {
+	msgBus := bus.NewMessageBus()
+	cfg := config.SlackConfig{
+		BotToken:  "xoxb-test",
+		AppToken:  "xapp-test",
+		AllowFrom: []string{"U_ALLOWED"},
+	}
+
+	ch, err := NewSlackChannel(cfg, msgBus)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	ch.botUserID = "U_BOT"
+	ch.api = nil
+
+	ev := &slackevents.AppMentionEvent{
+		User:      "U_BLOCKED",
+		Channel:   "C123456",
+		TimeStamp: "1700000000.000001",
+		Text:      "<@U_BOT> hello",
+	}
+
+	ch.handleAppMention(ev)
+
+	chatID := "C123456/1700000000.000001"
+	if _, ok := ch.pendingAcks.Load(chatID); ok {
+		t.Fatalf("blocked user should not create pending ack for chat %s", chatID)
+	}
+
+	assertNoInboundMessage(t, msgBus)
+}
+
+func TestSlackSlashCommandRejectsUsersOutsideAllowlist(t *testing.T) {
+	msgBus := bus.NewMessageBus()
+	cfg := config.SlackConfig{
+		BotToken:  "xoxb-test",
+		AppToken:  "xapp-test",
+		AllowFrom: []string{"U_ALLOWED"},
+	}
+
+	ch, err := NewSlackChannel(cfg, msgBus)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	event := socketmode.Event{
+		Data: slack.SlashCommand{
+			UserID:    "U_BLOCKED",
+			ChannelID: "C123456",
+			Command:   "/picoclaw",
+			Text:      "hello",
+		},
+	}
+
+	ch.handleSlashCommand(event)
+
+	assertNoInboundMessage(t, msgBus)
+}
+
+func assertNoInboundMessage(t *testing.T, msgBus *bus.MessageBus) {
+	t.Helper()
+
+	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+	defer cancel()
+
+	if msg, ok := msgBus.ConsumeInbound(ctx); ok {
+		t.Fatalf("expected no inbound message, got %+v", msg)
+	}
+}

pkg/channels/wecom.go

@@ -37,7 +37,7 @@ type WeComBotChannel struct {
 	ctx           context.Context
 	cancel        context.CancelFunc
 	processedMsgs map[string]bool // Message deduplication: msg_id -> processed
-	msgMu         sync.RWMutex
+	msgMu         *sync.RWMutex
 }
 
 // WeComBotMessage represents the JSON message structure from WeCom Bot (AIBOT)
@@ -102,6 +102,7 @@ func NewWeComBotChannel(cfg config.WeComConfig, messageBus *bus.MessageBus) (*We
 		BaseChannel:   base,
 		config:        cfg,
 		processedMsgs: make(map[string]bool),
+		msgMu:         &sync.RWMutex{},
 	}, nil
 }
 
@@ -330,6 +331,12 @@ func (c *WeComBotChannel) processMessage(ctx context.Context, msg WeComBotMessag
 
 	// Message deduplication: Use msg_id to prevent duplicate processing
 	msgID := msg.MsgID
+	if c.msgMu == nil {
+		c.msgMu = &sync.RWMutex{}
+	}
+	if c.processedMsgs == nil {
+		c.processedMsgs = make(map[string]bool)
+	}
 	c.msgMu.Lock()
 	if c.processedMsgs[msgID] {
 		c.msgMu.Unlock()
@@ -339,14 +346,15 @@ func (c *WeComBotChannel) processMessage(ctx context.Context, msg WeComBotMessag
 		return
 	}
 	c.processedMsgs[msgID] = true
-	c.msgMu.Unlock()
 
-	// Clean up old messages periodically (keep last 1000)
+	// Clean up old messages periodically (keep last 1000).
+	// Keep the current message id in the new map to preserve dedup behavior.
 	if len(c.processedMsgs) > 1000 {
-		c.msgMu.Lock()
-		c.processedMsgs = make(map[string]bool)
-		c.msgMu.Unlock()
+		c.processedMsgs = map[string]bool{
+			msgID: true,
+		}
 	}
+	c.msgMu.Unlock()
 
 	senderID := msg.From.UserID
 

pkg/config/config.go

@@ -8,6 +8,8 @@ import (
 	"sync/atomic"
 
 	"github.com/caarlos0/env/v11"
+
+	"github.com/sipeed/picoclaw/pkg/utils"
 )
 
 // rrCounter is a global counter for round-robin load balancing across models.
@@ -531,7 +533,7 @@ func SaveConfig(path string, cfg *Config) error {
 		return err
 	}
 
-	return os.WriteFile(path, data, 0o600)
+	return utils.WritePrivateFile(path, data)
 }
 
 func (c *Config) WorkspacePath() string {

pkg/cron/service.go

@@ -12,6 +12,8 @@ import (
 	"time"
 
 	"github.com/adhocore/gronx"
+
+	"github.com/sipeed/picoclaw/pkg/utils"
 )
 
 type CronSchedule struct {
@@ -340,7 +342,7 @@ func (cs *CronService) saveStoreUnsafe() error {
 		return err
 	}
 
-	return os.WriteFile(cs.storePath, data, 0o600)
+	return utils.WritePrivateFile(cs.storePath, data)
 }
 
 func (cs *CronService) AddJob(

pkg/cron/service_test.go

@@ -33,6 +33,41 @@ func TestSaveStore_FilePermissions(t *testing.T) {
 	}
 }
 
+func TestSaveStoreFixesExistingFilePermissions(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("file permission bits are not enforced on Windows")
+	}
+
+	storePath := filepath.Join(t.TempDir(), "cron", "jobs.json")
+	if err := os.MkdirAll(filepath.Dir(storePath), 0o755); err != nil {
+		t.Fatalf("failed to create store dir: %v", err)
+	}
+	if err := os.WriteFile(storePath, []byte(`{"version":1,"jobs":[]}`), 0o644); err != nil {
+		t.Fatalf("failed to create seed cron store: %v", err)
+	}
+
+	cs := NewCronService(storePath, nil)
+	if _, err := cs.AddJob(
+		"perm-test",
+		CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)},
+		"hello",
+		true,
+		"cli",
+		"direct",
+	); err != nil {
+		t.Fatalf("AddJob failed: %v", err)
+	}
+
+	info, err := os.Stat(storePath)
+	if err != nil {
+		t.Fatalf("stat cron store: %v", err)
+	}
+
+	if got := info.Mode().Perm(); got != 0o600 {
+		t.Fatalf("cron store perms = %o, want 600", got)
+	}
+}
+
 func int64Ptr(v int64) *int64 {
 	return &v
 }

pkg/utils/file.go

@@ -0,0 +1,11 @@
+package utils
+
+import "os"
+
+// WritePrivateFile writes data and enforces 0600 permissions for both new and existing files.
+func WritePrivateFile(path string, data []byte) error {
+	if err := os.WriteFile(path, data, 0o600); err != nil {
+		return err
+	}
+	return os.Chmod(path, 0o600)
+}