fix(perms): propagate sender/role through MCP bridge & delegate (#915)

cmd/gateway_consumer_normal.go

@@ -358,21 +358,40 @@ func processNormalMessage(
 
 	// Resolve effective sender: prefer MetaOriginSenderID when the on-wire
 	// SenderID is an internal/synthetic one (e.g. "notification:progress",
-	// "ticker:system", "system:escalation", "session_send_tool"). This lets
+	// "ticker:system", "system:escalation", "session_send_tool") OR is
+	// empty (some publish paths drop SenderID even though the originating
+	// turn carried a real user via MetaOriginSenderID). This lets
 	// system-initiated turns that DO have a real user behind them (because
 	// they propagated the origin via metadata) attribute actions to that user
-	// — e.g. for CheckFileWriterPermission in group chats (#915). Synthetic
-	// senders without propagation keep their on-wire value and hit F1's
-	// deny-in-group rule (safe default).
+	// — e.g. for CheckFileWriterPermission / CheckCronPermission in group
+	// chats (#915). Synthetic / empty senders without propagation keep their
+	// on-wire value and hit F1's deny-in-group rule (safe default).
 	effectiveSenderID := msg.SenderID
-	if bus.IsInternalSender(effectiveSenderID) {
+	if effectiveSenderID == "" || bus.IsInternalSender(effectiveSenderID) {
 		// Defense-in-depth: if a propagation bug ever writes a synthetic
 		// value into MetaOriginSenderID, do NOT honour it. We want only real
-		// user senders to override the on-wire synthetic.
+		// user senders to override the on-wire synthetic/empty.
 		if realSender := msg.Metadata[tools.MetaOriginSenderID]; realSender != "" && !bus.IsInternalSender(realSender) {
 			effectiveSenderID = realSender
 		}
 	}
+	// Diagnostic: warn when we end up with an empty sender for a group
+	// context. This is almost always a propagation bug somewhere upstream
+	// (channel handler, re-ingress publisher, or skill that lost ctx).
+	// CheckFileWriterPermission / CheckCronPermission will deny this turn.
+	if effectiveSenderID == "" && peerKind == string(sessions.PeerGroup) {
+		mk := make([]string, 0, len(msg.Metadata))
+		for k := range msg.Metadata {
+			mk = append(mk, k)
+		}
+		slog.Warn("group inbound has empty SenderID — group-scoped permission checks will deny",
+			"channel", msg.Channel,
+			"chat_id", msg.ChatID,
+			"on_wire_sender", msg.SenderID,
+			"meta_origin_sender", msg.Metadata[tools.MetaOriginSenderID],
+			"meta_keys", mk,
+		)
+	}
 	// Role propagation: carry the RBAC role of the originating actor so
 	// permission checks during the re-ingress turn can bypass per-user
 	// grants for authenticated admins (#915). Only present when the

cmd/gateway_managed.go

@@ -420,11 +420,26 @@ func wireExtras(
 				SessionKey:    sessionKey,
 				Message:       req.Task,
 				UserID:        req.UserID,
-				Channel:       "delegate",
+				// Propagate real acting sender + RBAC role through delegate so that
+				// downstream group-scoped permission checks (CheckCronPermission /
+				// CheckFileWriterPermission) attribute to the original user instead
+				// of an empty principal. Without these two lines, any cron/file
+				// mutation inside a delegated turn in a group context fails with
+				// `senderID=""` because loop_context.injectContext skips
+				// WithSenderID when req.SenderID is empty (#915 follow-up).
+				SenderID:      req.SenderID,
+				Role:          req.Role,
+				Channel:       req.Channel,
+				ChannelType:   "", // resolver not available here; consumer-only field
+				ChatID:        req.ChatID,
+				PeerKind:      req.PeerKind,
 				RunKind:       "delegate",
 				DelegationID:  req.DelegationID,
 				ParentAgentID: req.FromAgentKey,
 			}
+			if runReq.Channel == "" {
+				runReq.Channel = "delegate"
+			}
 			result, err := loop.Run(delegateCtx, runReq)
 			if err != nil {
 				return tools.DelegateResult{}, err

internal/agent/loop_pipeline_callbacks.go

@@ -239,6 +239,16 @@ func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx c
 		if tid := store.TenantIDFromContext(ctx); tid != uuid.Nil {
 			chatReq.Options[providers.OptTenantID] = tid.String()
 		}
+		// Pass real acting sender + RBAC role through to MCP bridge headers so
+		// group-scoped permission checks (CheckCronPermission, CheckFileWriterPermission)
+		// see the original user when bridge tools (cron, write_file, ...) are
+		// called by external CLI providers (claude-cli, opencode-go) (#915).
+		if req.SenderID != "" {
+			chatReq.Options[providers.OptSenderID] = req.SenderID
+		}
+		if req.Role != "" {
+			chatReq.Options[providers.OptRole] = req.Role
+		}
 
 		// Reasoning decision: resolve effort level for thinking models (o3, DeepSeek-R1, Kimi).
 		reasoningDecision := providers.ResolveReasoningDecision(

internal/gateway/server.go

@@ -223,6 +223,11 @@ func bridgeContextMiddleware(gatewayToken string, agentStore store.AgentStore, n
 		workspace := r.Header.Get("X-Workspace")
 		localKey := r.Header.Get("X-Local-Key")
 		sessionKey := r.Header.Get("X-Session-Key")
+		// Sender + role headers carry the real acting user behind a bridge call so
+		// group-scoped permission checks (CheckCronPermission, CheckFileWriterPermission)
+		// can attribute to the original user instead of an empty principal (#915).
+		senderID := r.Header.Get("X-Sender-ID")
+		roleHdr := r.Header.Get("X-Role")
 
 		if agentIDStr != "" || userID != "" {
 			// Reject context headers when no gateway token — prevents unauthenticated impersonation.
@@ -234,9 +239,11 @@ func bridgeContextMiddleware(gatewayToken string, agentStore store.AgentStore, n
 			}
 
 			// Verify HMAC signature over all context fields.
+			// Extras order MUST match SignBridgeContext call in claude_cli_mcp.go:
+			//   localKey | sessionKey | senderID | role
 			tenantIDStr := r.Header.Get("X-Tenant-ID")
 			sig := r.Header.Get("X-Bridge-Sig")
-			ok, tenantVerified := providers.VerifyBridgeContext(gatewayToken, agentIDStr, userID, channel, chatID, peerKind, workspace, tenantIDStr, sig, localKey, sessionKey)
+			ok, tenantVerified := providers.VerifyBridgeContext(gatewayToken, agentIDStr, userID, channel, chatID, peerKind, workspace, tenantIDStr, sig, localKey, sessionKey, senderID, roleHdr)
 			if !ok {
 				slog.Warn("security.mcp_bridge: invalid bridge context signature",
 					"agent_id", agentIDStr, "user_id", userID)
@@ -264,6 +271,16 @@ func bridgeContextMiddleware(gatewayToken string, agentStore store.AgentStore, n
 			if userID != "" {
 				ctx = store.WithUserID(ctx, userID)
 			}
+			// Inject sender ID + role only when HMAC was valid (we're inside the
+			// `if ok { ... }` block from VerifyBridgeContext). These are used by
+			// group-scoped permission checks downstream — see CheckCronPermission /
+			// CheckFileWriterPermission. Without HMAC coverage we never trust these.
+			if senderID != "" {
+				ctx = store.WithSenderID(ctx, senderID)
+			}
+			if roleHdr != "" {
+				ctx = store.WithRole(ctx, roleHdr)
+			}
 			// Only inject tenant_id when HMAC actually covers it (level 1).
 			// Fallback levels (pre-tenantID sessions) must not trust unsigned tenant headers.
 			if tenantVerified && tenantIDStr != "" {

internal/providers/claude_cli.go

@@ -38,6 +38,18 @@ const OptTenantID = "tenant_id"
 // OptLocalKey passes the composite local key (e.g. "-100123:topic:42") for forum topic routing.
 const OptLocalKey = "local_key"
 
+// OptSenderID passes the real acting sender (e.g. raw Telegram user ID) for
+// per-user permission checks (CheckCronPermission, CheckFileWriterPermission)
+// on group-scoped tools called via MCP bridge. Without this, group cron/file
+// mutations fail with senderID="" because bridge middleware has no way to know
+// who actually triggered the turn (#915 follow-up).
+const OptSenderID = "sender_id"
+
+// OptRole passes the caller's RBAC role (admin/operator/owner/...) so bridge
+// permission checks can apply the admin bypass (#915) for tenant-authenticated
+// dispatches that route through MCP bridge.
+const OptRole = "role"
+
 // ClaudeCLIProvider implements Provider by shelling out to the `claude` CLI binary.
 // It acts as a thin proxy: CLI manages session history, tool execution, and context.
 // GoClaw only forwards the latest user message and streams back the response.

internal/providers/claude_cli_mcp.go

@@ -92,17 +92,19 @@ type BridgeContext struct {
 	Workspace string
 	TenantID  string
 	LocalKey  string
+	SenderID  string // real acting sender (#915) — needed by group-scoped permission checks
+	Role      string // caller RBAC role (#915) — admin/operator/owner bypass per-user grants
 }
 
 // WriteMCPConfig writes a per-session MCP config file with agent context headers.
 // Files are stored at ~/.goclaw/mcp-configs/<safe-session-key>/mcp-config.json,
 // outside the agent's workDir so tokens are not exposed.
 // Skips write if content is unchanged. Returns the file path.
 func (d *MCPConfigData) WriteMCPConfig(ctx context.Context, sessionKey string, bc BridgeContext) string {
-	return d.writeMCPConfigInternal(ctx, sessionKey, bc.AgentID, bc.UserID, bc.Channel, bc.ChatID, bc.PeerKind, bc.Workspace, bc.TenantID, bc.LocalKey)
+	return d.writeMCPConfigInternal(ctx, sessionKey, bc.AgentID, bc.UserID, bc.Channel, bc.ChatID, bc.PeerKind, bc.Workspace, bc.TenantID, bc.LocalKey, bc.SenderID, bc.Role)
 }
 
-func (d *MCPConfigData) writeMCPConfigInternal(ctx context.Context, sessionKey, agentID, userID, channel, chatID, peerKind, workspace, tenantID, localKey string) string {
+func (d *MCPConfigData) writeMCPConfigInternal(ctx context.Context, sessionKey, agentID, userID, channel, chatID, peerKind, workspace, tenantID, localKey, senderID, role string) string {
 	if d == nil || (len(d.Servers) == 0 && d.GatewayAddr == "" && d.AgentMCPLookup == nil) {
 		return ""
 	}
@@ -158,9 +160,17 @@ func (d *MCPConfigData) writeMCPConfigInternal(ctx context.Context, sessionKey,
 		if sessionKey != "" && !strings.ContainsAny(sessionKey, "\r\n\x00") {
 			headers["X-Session-Key"] = sessionKey
 		}
-		// HMAC signature over all context fields to prevent header forgery
+		if senderID != "" && !strings.ContainsAny(senderID, "\r\n\x00") {
+			headers["X-Sender-ID"] = senderID
+		}
+		if role != "" && !strings.ContainsAny(role, "\r\n\x00") {
+			headers["X-Role"] = role
+		}
+		// HMAC signature over all context fields to prevent header forgery.
+		// Order MUST match VerifyBridgeContext extras in bridgeContextMiddleware:
+		//   localKey | sessionKey | senderID | role
 		if d.GatewayToken != "" && (agentID != "" || userID != "") {
-			headers["X-Bridge-Sig"] = SignBridgeContext(d.GatewayToken, agentID, userID, channel, chatID, peerKind, workspace, tenantID, localKey, sessionKey)
+			headers["X-Bridge-Sig"] = SignBridgeContext(d.GatewayToken, agentID, userID, channel, chatID, peerKind, workspace, tenantID, localKey, sessionKey, senderID, role)
 		}
 
 		bridgeEntry := map[string]any{
@@ -279,14 +289,24 @@ func SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspac
 // Returns (ok, tenantVerified): ok indicates signature is valid, tenantVerified indicates
 // the tenantID field was covered by the HMAC (only true at level 1).
 // Falls back to old formats for backward compatibility with sessions whose MCP config
-// was written before the workspace or tenantID fields were added.
+// was written before the workspace, tenantID, or sender/role fields were added.
 // Callers must NOT trust the tenantID header when tenantVerified is false.
+//
+// Extras passed in current format (in order):
+//   localKey | sessionKey | senderID | role
 func VerifyBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID, sig string, extra ...string) (bool, bool) {
-	// Current format: all fields including localKey
+	// Current format: all fields including localKey, sessionKey, senderID, role
 	expected := SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID, extra...)
 	if hmac.Equal([]byte(expected), []byte(sig)) {
 		return true, true
 	}
+	// Fallback: pre-sender/role sessions — drop the last 2 extras (senderID, role).
+	if len(extra) >= 2 {
+		preSender := SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID, extra[:len(extra)-2]...)
+		if hmac.Equal([]byte(preSender), []byte(sig)) {
+			return true, true
+		}
+	}
 	// Fallback: without extra fields (pre-localKey sessions)
 	noExtra := SignBridgeContext(key, agentID, userID, channel, chatID, peerKind, workspace, tenantID)
 	if hmac.Equal([]byte(noExtra), []byte(sig)) {

internal/providers/claude_cli_session.go

@@ -178,6 +178,8 @@ func bridgeContextFromOpts(opts map[string]any) BridgeContext {
 		Workspace: extractStringOpt(opts, OptWorkspace),
 		TenantID:  extractStringOpt(opts, OptTenantID),
 		LocalKey:  extractStringOpt(opts, OptLocalKey),
+		SenderID:  extractStringOpt(opts, OptSenderID),
+		Role:      extractStringOpt(opts, OptRole),
 	}
 }
 

internal/store/config_permission_store.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"log/slog"
 	"strings"
 	"time"
 
@@ -139,7 +140,17 @@ func CheckCronPermission(ctx context.Context, permStore ConfigPermissionStore) e
 	}
 	senderID := SenderIDFromContext(ctx)
 	if senderID == "" || isSyntheticSender(senderID) {
-		return fmt.Errorf("permission denied: system context cannot manage cron jobs in group chats")
+		// Diagnostic log: emit full ctx fingerprint so admins can find the
+		// upstream caller that lost sender attribution.
+		slog.Warn("CheckCronPermission denied: no real sender",
+			"reason", "synthetic_or_empty_sender",
+			"sender_id", senderID,
+			"user_id", userID,
+			"agent_id", agentID.String(),
+			"role", RoleFromContext(ctx),
+			"tenant_id", TenantIDFromContext(ctx).String(),
+		)
+		return fmt.Errorf("permission denied: cron mutation requires a real user sender (got senderID=%q in scope=%q). System contexts (subagent/teammate/notification/ticker/system) cannot manage cron jobs in groups. Check goclaw.log for `CheckCronPermission denied` warning to identify the upstream caller losing sender attribution", senderID, userID)
 	}
 	numericID := strings.SplitN(senderID, "|", 2)[0]
 
@@ -157,7 +168,19 @@ func CheckCronPermission(ctx context.Context, permStore ConfigPermissionStore) e
 		return nil // fail-open
 	}
 	if !allowed {
-		return fmt.Errorf("permission denied: only users with cron or file_writer permission can manage cron jobs in group chats")
+		slog.Warn("CheckCronPermission denied: no grant",
+			"reason", "no_grant",
+			"sender_id", numericID,
+			"user_id", userID,
+			"agent_id", agentID.String(),
+			"checked_types", []string{ConfigTypeCron, ConfigTypeFileWriter},
+		)
+		return fmt.Errorf(
+			"permission denied: cron requires a `%s` or `%s` grant — none found for user=%s in scope=%q. "+
+				"Fix: ask an existing writer to run `/addcron` (reply to your message) in this group, "+
+				"or `/addwriter` for full file_writer access. Verify with `/croners` or `/writers`.",
+			ConfigTypeCron, ConfigTypeFileWriter, numericID, userID,
+		)
 	}
 	return nil
 }

internal/tools/cron.go

@@ -145,7 +145,10 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *Result {
 	// Group cron permission check for mutation actions
 	if t.permStore != nil && (action == "add" || action == "update" || action == "remove") {
 		if err := store.CheckCronPermission(ctx, t.permStore); err != nil {
-			return ErrorResult("permission denied: only users with cron or file_writer permission can manage cron jobs in group chats")
+			// Surface the diagnostic-rich message from CheckCronPermission so
+			// callers (LLMs, users) know exactly which user/scope/configType
+			// was missing instead of a generic "permission denied".
+			return ErrorResult(err.Error())
 		}
 	}
 

internal/tools/team_tool_dispatch.go

@@ -164,6 +164,21 @@ func (m *TeamToolManager) dispatchTaskToAgent(ctx context.Context, task *store.T
 		}
 	}
 
+	// Diagnostic: if we still have no real sender and the dispatch lands in
+	// a group, downstream group-scoped permission checks (cron, file_writer)
+	// will deny. Log so admins can identify the upstream source.
+	if (originSenderID == "" || bus.IsInternalSender(originSenderID)) && originPeerKind == "group" {
+		slog.Warn("team dispatch: no real sender for group target — group permissions will deny",
+			"task_id", task.ID.String(),
+			"team_id", teamID.String(),
+			"to_agent", ag.AgentKey,
+			"ctx_sender", store.SenderIDFromContext(ctx),
+			"task_meta_sender", task.Metadata["origin_sender_id"],
+			"peer_kind", originPeerKind,
+			"chat_id", originChatID,
+		)
+	}
+
 	meta := map[string]string{
 		MetaOriginChannel:   originChannel,
 		MetaOriginPeerKind:  originPeerKind,