diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 000000000..aa8fbcb4a --- /dev/null +++ b/.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 ./... diff --git a/pkg/auth/store_test.go b/pkg/auth/store_test.go index f6793cfce..edd84762c 100644 --- a/pkg/auth/store_test.go +++ b/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 { diff --git a/pkg/channels/slack.go b/pkg/channels/slack.go index f7359cd6d..5df62896e 100644 --- a/pkg/channels/slack.go +++ b/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 diff --git a/pkg/channels/slack_test.go b/pkg/channels/slack_test.go index 3707c2703..36da6adfe 100644 --- a/pkg/channels/slack_test.go +++ b/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) + } +} diff --git a/pkg/channels/wecom.go b/pkg/channels/wecom.go index f8daf89de..b3edbcf44 100644 --- a/pkg/channels/wecom.go +++ b/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 diff --git a/pkg/config/config.go b/pkg/config/config.go index 2595398c7..ff8f3a1b8 100644 --- a/pkg/config/config.go +++ b/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 { diff --git a/pkg/cron/service.go b/pkg/cron/service.go index e699a44b5..98a39efd4 100644 --- a/pkg/cron/service.go +++ b/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( diff --git a/pkg/cron/service_test.go b/pkg/cron/service_test.go index 1a0dd1829..be1997b29 100644 --- a/pkg/cron/service_test.go +++ b/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 } diff --git a/pkg/utils/file.go b/pkg/utils/file.go new file mode 100644 index 000000000..8849c7f64 --- /dev/null +++ b/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) +}