Fix #179 security hardening and #199 default model/CI by jmahotiedu · Pull Request #211 · sipeed/picoclaw

jmahotiedu · 2026-02-15T12:30:46Z

Summary

fix Slack allowlist bypass for app mentions and slash commands (Slack allow list bypass + a few other security fixes #179)
harden secret/sensitive file persistence to private permissions (

Leeaandrob · 2026-02-16T19:27:25Z

@Zepan This PR addresses security hardening (fixing #179) and default model/CI improvements (#199). The roadmap explicitly calls for Security Audit & Bug Fixes as volunteer role #3.

Recommendation: Review and merge. Security fixes should be prioritized — the roadmap document acknowledges early "vibe coding" created security gaps.

Leeaandrob · 2026-02-17T15:56:27Z

@jmahotiedu take look into the conflicts

Leeaandrob

PR #211 Deep Review — @jmahotiedu

Excellent work, @jmahotiedu. This is a well-structured security PR that addresses real vulnerabilities from issue #179 with proper test coverage. The migration strategy for the default model is smart — gracefully handles existing users' configs at load time.

Verification

Checked out branch locally: fix/issues-179-199-security-model
go vet ./pkg/channels/... ./pkg/config/... ./pkg/cron/... ./pkg/auth/... ./pkg/migrate/... — clean
go test on all 5 modified packages — all 62 tests passing
go test ./pkg/channels/... -race — clean, no data races
Confirmed handleAppMention, handleSlashCommand, and handleMessageEvent ALL now have IsAllowed() checks
Confirmed openrouter/auto is correctly routed via strings.HasPrefix(model, "openrouter/") at http_provider.go:203
Confirmed writePrivateFile correctly handles both new files (via 0600 perm) AND existing files with wrong perms (via explicit os.Chmod)
Audited issue #179 — items 1 (file perms) and 2 (Slack allowlist) are fixed; item 3 (cron exec restriction) appears to have been refactored away in current code architecture

Summary of Findings

MEDIUM (Suggestions, not blocking)

M1: writePrivateFile is duplicated identically in config.go:323 and cron/service.go:460 — DRY violation. Move to pkg/utils/.
M2: normalizeLegacyModelDefaults — no test for the edge case where glm-4.7 is set but NEITHER Zhipu nor OpenRouter key exists (model stays glm-4.7 with no usable provider). Add a test to document this intentional behavior.
M3: CI workflow could benefit from go vet ./... step (catches common Go issues beyond formatting) and -race flag on go test.

LOW

L1: Merge conflicts with current upstream/main are expected (Chinese/Japanese READMEs don't exist on this branch's base). Already flagged.

POSITIVE

Slack allowlist bypass fixed — handleAppMention (line 298) and handleSlashCommand (line 355) now enforce IsAllowed() BEFORE any processing (reactions, acks, HandleMessage). Correct placement.
File permissions hardened to 0600 — config.json and cron store both contain API keys/tokens. 0644 was a real leak vector on shared systems.
Smart migration — normalizeLegacyModelDefaults at config load time gracefully migrates existing users without breaking Zhipu users who intentionally use glm-4.7.
Test coverage — every change has corresponding tests: allowlist rejection for app mentions and slash commands, file permission verification, config normalization (both OpenRouter migration AND Zhipu preservation).
Cross-platform test awareness — runtime.GOOS == "windows" skip for permission tests; setTestHome helper for HOME env across Linux/Mac/Windows.
CI workflow — clean gofmt + go test, go-version-file: go.mod for version consistency.
os.Chmod after os.WriteFile is correct — WriteFile doesn't change permissions on existing files, so the chmod is necessary for files that already exist with 0644.

Verdict: APPROVE

Solid security hardening with comprehensive tests. The medium items are suggestions for improvement, not blockers. Ready to merge after resolving upstream merge conflicts.

Leeaandrob · 2026-02-17T16:03:17Z

pkg/channels/slack.go

 	if ev.User == c.botUserID {
 		return
 	}
+	if !c.IsAllowed(ev.User) {


[Positive — Allowlist Fix for App Mentions] @jmahotiedu — Correct placement. The IsAllowed() check is placed BEFORE the reaction emoji, pending ack, and HandleMessage() call. This means blocked users get zero feedback — no "eyes" reaction, no processing. This is the right security posture.

Note: BaseChannel.HandleMessage (base.go:86) also has its own IsAllowed() check as defense-in-depth, but this early return avoids unnecessary work (reaction API calls, string processing) for blocked users.

Leeaandrob · 2026-02-17T16:03:17Z

pkg/channels/slack.go

+	if !c.IsAllowed(senderID) {
+		logger.DebugCF("slack", "Slash command rejected by allowlist", map[string]interface{}{
+			"user_id": senderID,
+			"command": cmd.Command,


[Positive — Allowlist Fix for Slash Commands] @jmahotiedu — Same correct pattern as app mentions. The check is early, before any processing or response. The cmd.Command in the debug log is a good touch for audit trails.

Leeaandrob · 2026-02-17T16:03:18Z

pkg/config/config.go

+	return os.Chmod(path, 0600)
+}
+
+func normalizeLegacyModelDefaults(cfg *Config) {


[Positive + M2 Suggestion] @jmahotiedu — Smart migration function. The logic flow is clear:

Not glm-4.7? → leave it alone (user chose something deliberately)

Has Zhipu key? → leave it (they intend to use Zhipu)

Has OpenRouter key? → migrate to openrouter/auto

Neither key? → do nothing (model stays glm-4.7)

Case 4 is intentionally conservative (we cannot guess the user's intended provider), but it should have a test to document this behavior:

func TestLoadConfigKeepsGLMWhenNoKeysConfigured(t *testing.T) { // ... config with model=glm-4.7, no API keys // Assert model is still glm-4.7 }

Addressed in e2f3b43: added TestLoadConfigKeepsLegacyGLMModelWhenNoProviderKeysConfigured in pkg/config/config_test.go to document the no-provider-keys glm-4.7 behavior.

Leeaandrob · 2026-02-17T16:03:18Z

pkg/config/config.go

 	return path
 }
+
+func writePrivateFile(path string, data []byte) error {


[M1 — MEDIUM: DRY Violation] @jmahotiedu — This exact function is duplicated in pkg/cron/service.go:460. Both are identical:

func writePrivateFile(path string, data []byte) error { if err := os.WriteFile(path, data, 0600); err != nil { return err } return os.Chmod(path, 0600) }

Consider moving it to pkg/utils/ (which already exists with media.go and string.go) and importing from both packages. Not blocking, but worth cleaning up.

Addressed in e2f3b43: moved the duplicated helper to pkg/utils/file.go as utils.WritePrivateFile and switched both config/cron call sites to use it.

Leeaandrob · 2026-02-17T21:39:21Z

@jmahotiedu take look on it

… coverage Move duplicated writePrivateFile logic into pkg/utils.WritePrivateFile, reuse it from config and cron, add the glm-4.7 no-provider-keys test, and extend CI with go vet plus race-enabled tests.

jmahotiedu · 2026-02-18T19:36:11Z

Addressed the review suggestions in follow-up commit e2f3b43:

M1: deduplicated private-file write logic by introducing pkg/utils.WritePrivateFile and using it from both pkg/config/config.go and pkg/cron/service.go.
M2: added the missing edge-case test TestLoadConfigKeepsLegacyGLMModelWhenNoProviderKeysConfigured in pkg/config/config_test.go.
M3: updated CI workflow (.github/workflows/ci.yml) to run go vet ./... and go test -race ./....

Local verification run:

go test ./...
go test ./pkg/config ./pkg/cron ./pkg/utils
go vet ./pkg/config ./pkg/cron ./pkg/utils

Note: go test -race could not be executed in my local Windows environment because CGO is disabled by default there; race coverage is now enforced in CI on Linux.

Resolve config/cron merge conflicts while preserving security hardening and follow-up review fixes: shared private-file writer, glm legacy-model normalization tests, and CI vet/race checks.

jmahotiedu · 2026-02-18T19:46:46Z

Follow-up: merged upstream main into this branch in 11f4a51, resolved the remaining conflicts, and preserved the review-fix changes (utils.WritePrivateFile, legacy-model normalization tests, and CI vet/race checks). The PR is now mergeable again.

Run go generate ./cmd/picoclaw on CI so //go:embed workspace assets exist before go vet and race tests.

jmahotiedu · 2026-02-18T19:48:39Z

Addressed the CI failure in ee798bd by generating embedded workspace assets before vet/tests (go generate ./cmd/picoclaw), so go vet ./... no longer fails on missing workspace embed files in clean CI checkouts.

jmahotiedu · 2026-02-23T18:19:16Z

@Leeaandrob @Zepan

Conflicts are resolved and PR #211 is now mergeable (mergeable_state: clean). I pushed the updates to the PR branch (efb6850).

What was done:

Merged latest main into fix/issues-179-199-security-model
Resolved conflicts in README.md, config/config.example.json, pkg/config/config.go, pkg/cron/service.go
Kept security hardening behavior (Slack allowlist checks + private-file writes via utils.WritePrivateFile)
Fixed follow-up lint/format issues from the merge

Local verification:

go test ./pkg/channels ./pkg/config ./pkg/auth ./pkg/cron ./pkg/migrate (pass)

Fresh CI is running for this head:

PR workflow: https://github.com/sipeed/picoclaw/actions/runs/22319073695
CI workflow: https://github.com/sipeed/picoclaw/actions/runs/22319073787

If these complete green, please merge ASAP.

jmahotiedu · 2026-02-23T18:26:17Z

@Leeaandrob @Zepan

Update: latest head 8da7f08 is now fully green.

PR workflow: ✅ https://github.com/sipeed/picoclaw/actions/runs/22319254133
CI workflow: ✅ https://github.com/sipeed/picoclaw/actions/runs/22319256328

PR is conflict-free and mergeable (mergeable_state: clean). Please merge when convenient.

jmahotiedu · 2026-02-23T18:35:09Z

@Huaaudio @Leeaandrob quick status update for #211:

The WeCom race-test failure is fixed on the current head (8da7f08), and the latest checks are all green:

CI / test: https://github.com/sipeed/picoclaw/actions/runs/22319256328
PR / Linter + Tests: https://github.com/sipeed/picoclaw/actions/runs/22319254133

GitHub now shows this PR as blocked on review (REVIEW_REQUIRED).
Could one of you please do the final review and merge if everything looks good?

jmahotiedu added 2 commits February 15, 2026 07:30

fix: address sipeed#179 and sipeed#199 with tests

1e966a0

ci: add gofmt and go test workflow

3f66ccb

This was referenced Feb 15, 2026

LLM call fails with 400 error: glm-4.7 is not a valid model ID #199

Closed

Slack allow list bypass + a few other security fixes #179

Closed

Leeaandrob approved these changes Feb 17, 2026

View reviewed changes

refactor: centralize private file writes and tighten config migration…

e2f3b43

… coverage Move duplicated writePrivateFile logic into pkg/utils.WritePrivateFile, reuse it from config and cron, add the glm-4.7 no-provider-keys test, and extend CI with go vet plus race-enabled tests.

Merge origin/main into fix/issues-179-199-security-model

11f4a51

Resolve config/cron merge conflicts while preserving security hardening and follow-up review fixes: shared private-file writer, glm legacy-model normalization tests, and CI vet/race checks.

ci: generate embedded workspace before vet and tests

ee798bd

Run go generate ./cmd/picoclaw on CI so //go:embed workspace assets exist before go vet and race tests.

jmahotiedu closed this Feb 19, 2026

jmahotiedu reopened this Feb 19, 2026

jmahotiedu added 2 commits February 23, 2026 13:12

merge: sync origin/main and resolve PR sipeed#211 conflicts

9c96618

chore: fix lint formatting after conflict resolution

efb6850

jmahotiedu added 2 commits February 23, 2026 13:21

fix(wecom): guard dedup map cleanup under mutex

a37b3ba

fix: avoid wecom message dedupe race in async callback

8da7f08

Conversation

jmahotiedu commented Feb 15, 2026

Summary

Uh oh!

Leeaandrob commented Feb 16, 2026

Uh oh!

Leeaandrob commented Feb 17, 2026

Uh oh!

Leeaandrob left a comment

Choose a reason for hiding this comment

PR #211 Deep Review — @jmahotiedu

Verification

Summary of Findings

Verdict: APPROVE

Uh oh!

Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

jmahotiedu Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

jmahotiedu Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Leeaandrob commented Feb 17, 2026

Uh oh!

jmahotiedu commented Feb 18, 2026

Uh oh!

jmahotiedu commented Feb 18, 2026

Uh oh!

jmahotiedu commented Feb 18, 2026

Uh oh!

jmahotiedu commented Feb 23, 2026

Uh oh!

jmahotiedu commented Feb 23, 2026

Uh oh!

jmahotiedu commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants