From b20d051eae7d535ce2a57bcce2ea19cdffefaed1 Mon Sep 17 00:00:00 2001 From: mozaa Date: Wed, 6 May 2026 16:38:50 +0700 Subject: [PATCH] fix(perms): propagate sender/role through MCP bridge & delegate (#915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Group-scoped permission checks (CheckCronPermission / CheckFileWriterPermission) fail with senderID="" when tools are invoked through: 1. MCP bridge (external CLI providers like claude-cli) — middleware never injected SenderID/Role into ctx, so even though the agent loop set them on RunRequest, the bridge layer dropped attribution. 2. delegate tool (gateway_managed.go delegateRunFn) — built RunRequest without copying SenderID/Role/Channel/ChatID/PeerKind from DelegateRequest, so the target agent's loop_context.injectContext skipped WithSenderID and downstream group cron/file_writer mutations denied with senderID="". Fix: - claude_cli: new OptSenderID, OptRole; BridgeContext carries SenderID + Role; writeMCPConfig emits X-Sender-ID / X-Role headers and signs them in the HMAC payload (extras: localKey | sessionKey | senderID | role). VerifyBridgeContext gains a backward-compat fallback that drops the last 2 extras for sessions written pre-this-change. - gateway/server.bridgeContextMiddleware: read X-Sender-ID + X-Role headers (HMAC-protected), inject WithSenderID / WithRole into ctx. - agent loop_pipeline_callbacks: pass req.SenderID and req.Role into chatReq.Options so external CLI providers receive them. - gateway_managed.delegateRunFn: propagate SenderID/Role/Channel/ChatID/ PeerKind from DelegateRequest into RunRequest. - team_tool_dispatch / gateway_consumer_normal: warn when sender is empty in a group-target dispatch so admins can find the upstream caller losing attribution. - config_permission_store: surface diagnostic-rich error messages so LLMs/users know exactly which user/scope/configType denied, instead of a generic "permission denied". - cron tool: forward CheckCronPermission's diagnostic message to the LLM result so the agent can self-correct. --- cmd/gateway_consumer_normal.go | 31 +++++++++++++++++----- cmd/gateway_managed.go | 17 +++++++++++- internal/agent/loop_pipeline_callbacks.go | 10 +++++++ internal/gateway/server.go | 19 +++++++++++++- internal/providers/claude_cli.go | 12 +++++++++ internal/providers/claude_cli_mcp.go | 32 ++++++++++++++++++----- internal/providers/claude_cli_session.go | 2 ++ internal/store/config_permission_store.go | 27 +++++++++++++++++-- internal/tools/cron.go | 5 +++- internal/tools/team_tool_dispatch.go | 15 +++++++++++ 10 files changed, 153 insertions(+), 17 deletions(-) diff --git a/cmd/gateway_consumer_normal.go b/cmd/gateway_consumer_normal.go index 59c7e6e850..8aff5a090f 100644 --- a/cmd/gateway_consumer_normal.go +++ b/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 diff --git a/cmd/gateway_managed.go b/cmd/gateway_managed.go index 5d2c111ba2..1b2c1283c2 100644 --- a/cmd/gateway_managed.go +++ b/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 diff --git a/internal/agent/loop_pipeline_callbacks.go b/internal/agent/loop_pipeline_callbacks.go index cdbfc1c53b..d4fdb32df3 100644 --- a/internal/agent/loop_pipeline_callbacks.go +++ b/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( diff --git a/internal/gateway/server.go b/internal/gateway/server.go index cbfb79fcd2..c78b839d5a 100644 --- a/internal/gateway/server.go +++ b/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 != "" { diff --git a/internal/providers/claude_cli.go b/internal/providers/claude_cli.go index e5e0dde71b..f89cff6a94 100644 --- a/internal/providers/claude_cli.go +++ b/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. diff --git a/internal/providers/claude_cli_mcp.go b/internal/providers/claude_cli_mcp.go index df03d0808a..9a502555be 100644 --- a/internal/providers/claude_cli_mcp.go +++ b/internal/providers/claude_cli_mcp.go @@ -92,6 +92,8 @@ 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. @@ -99,10 +101,10 @@ type BridgeContext struct { // 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)) { diff --git a/internal/providers/claude_cli_session.go b/internal/providers/claude_cli_session.go index 452bfc0f5a..236d0447cf 100644 --- a/internal/providers/claude_cli_session.go +++ b/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), } } diff --git a/internal/store/config_permission_store.go b/internal/store/config_permission_store.go index 56f5ccbbb8..0183bc3940 100644 --- a/internal/store/config_permission_store.go +++ b/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 } diff --git a/internal/tools/cron.go b/internal/tools/cron.go index 32b96a4395..92a309fb1a 100644 --- a/internal/tools/cron.go +++ b/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()) } } diff --git a/internal/tools/team_tool_dispatch.go b/internal/tools/team_tool_dispatch.go index 3cfafc2742..7cbf3e3aee 100644 --- a/internal/tools/team_tool_dispatch.go +++ b/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,