diff --git a/internal/gateway/router.go b/internal/gateway/router.go index c6fa0d65..6e994a8a 100644 --- a/internal/gateway/router.go +++ b/internal/gateway/router.go @@ -67,12 +67,25 @@ func (r *MethodRouter) Handle(ctx context.Context, client *Client, req *protocol if req.Method != protocol.MethodConnect && req.Method != protocol.MethodHealth && req.Method != protocol.MethodBrowserPairingStatus { if pe := r.server.policyEngine; pe != nil { if !pe.CanAccess(client.role, req.Method) { - slog.Warn("permission denied", "method", req.Method, "role", client.role, "client", client.id) + required := permissions.MethodRole(req.Method) + slog.Warn("security.permission_denied", + "method", req.Method, + "role", client.role, + "required", string(required), + "client", client.id, + ) locale := i18n.Normalize(client.locale) + var msg string + if required == permissions.RoleNone { + // Unclassified method — fail-closed (issue #866 fix). + msg = i18n.T(locale, i18n.MsgPermissionDenied, req.Method+" is not permitted for this session") + } else { + msg = i18n.T(locale, i18n.MsgPermissionDenied, req.Method+" requires "+string(required)+" role") + } client.SendResponse(protocol.NewErrorResponse( req.ID, protocol.ErrUnauthorized, - i18n.T(locale, i18n.MsgPermissionDenied, req.Method+" requires "+string(permissions.MethodRole(req.Method))+" role"), + msg, )) return } @@ -286,17 +299,22 @@ func (r *MethodRouter) handleConnect(ctx context.Context, client *Client, req *p } } - // Path 4: Fallback → viewer (wrong token or pairing not available) - client.role = permissions.RoleViewer - client.authenticated = true - client.userID = params.UserID - tid, errCode := r.resolveTenantHint(ctx, params.TenantHint, params.UserID) - if errCode != "" { - client.SendResponse(protocol.NewErrorResponse(req.ID, errCode, "tenant access revoked")) - return - } - client.tenantID = tid - r.sendConnectResponse(ctx, client, req.ID) + // Path 4: Fail-closed — no valid token, no valid pairing → reject. + // Previously fell back to viewer+authenticated=true, which allowed any + // unauthenticated client to exercise the default-permit policy (CVE #866). + slog.Warn("security.ws_connect_rejected", + "reason", "no_valid_credentials", + "client", client.id, + "has_token", params.Token != "", + "has_sender_id", params.SenderID != "", + ) + client.authenticated = false + locale := i18n.Normalize(client.locale) + client.SendResponse(protocol.NewErrorResponse( + req.ID, + protocol.ErrUnauthorized, + i18n.T(locale, i18n.MsgPermissionDenied, "valid token or active pairing required"), + )) } func (r *MethodRouter) sendConnectResponse(ctx context.Context, client *Client, reqID string) { diff --git a/internal/permissions/policy.go b/internal/permissions/policy.go index d5227d8c..5e4f391a 100644 --- a/internal/permissions/policy.go +++ b/internal/permissions/policy.go @@ -27,6 +27,11 @@ const ( RoleAdmin Role = "admin" // Full access to all methods RoleOperator Role = "operator" // Read + write access (no admin operations) RoleViewer Role = "viewer" // Read-only access + + // RoleNone is a sentinel returned by MethodRole for methods that have no + // explicit classification. The router treats it as deny-for-everyone so + // newly-added RPCs are secure-by-default (fail-closed). + RoleNone Role = "" ) // Scope represents a specific permission scope. @@ -88,8 +93,14 @@ func (pe *PolicyEngine) IsOwner(senderID string) bool { } // CanAccess checks if a role has access to a gateway RPC method. +// Unclassified methods (MethodRole == RoleNone) are denied for every role +// including owner — callers must surface an explicit PERMISSION_DENIED/ +// UNAUTHORIZED and never silently permit. func (pe *PolicyEngine) CanAccess(role Role, method string) bool { requiredRole := MethodRole(method) + if requiredRole == RoleNone { + return false + } return roleLevel(role) >= roleLevel(requiredRole) } @@ -130,7 +141,18 @@ func RoleFromScopes(scopes []Scope) Role { } // MethodRole returns the minimum role required for a given RPC method. +// +// Policy is fail-closed (default-deny): methods absent from every allowlist +// return RoleNone. The gateway dispatcher must reject RoleNone with an +// UNAUTHORIZED / PERMISSION_DENIED error rather than granting viewer access. +// This is the fix for issue #866 where default-permit let unauthenticated +// clients invoke mutation/exfiltration RPCs (heartbeat.*, logs.tail, etc.). func MethodRole(method string) Role { + // System methods that bypass auth entirely (pre-auth handshake only). + if isPublicMethod(method) { + return RoleViewer + } + // Admin-only methods if isAdminMethod(method) { return RoleAdmin @@ -141,8 +163,26 @@ func MethodRole(method string) Role { return RoleOperator } - // Everything else is read-only (viewer can access) - return RoleViewer + // Read-only methods (viewer and above) + if isReadMethod(method) { + return RoleViewer + } + + // Fail-closed: unknown / unclassified method → deny. + return RoleNone +} + +// isPublicMethod lists methods every authenticated (and some pre-auth) client +// is allowed to call. Kept tiny on purpose — do not expand without review. +func isPublicMethod(method string) bool { + switch method { + case protocol.MethodConnect, + protocol.MethodHealth, + protocol.MethodStatus, + protocol.MethodBrowserPairingStatus: + return true + } + return false } // MethodScopes returns the scopes required for a method. @@ -164,38 +204,91 @@ func MethodScopes(method string) []Scope { func isAdminMethod(method string) bool { adminMethods := []string{ + // Config — admin/owner only. Additional master-scope/owner guards live + // in the handler middleware, but classify here for defense-in-depth. + protocol.MethodConfigGet, protocol.MethodConfigApply, protocol.MethodConfigPatch, + protocol.MethodConfigSchema, + protocol.MethodConfigDefaults, + protocol.MethodConfigPermissionsList, + protocol.MethodConfigPermissionsGrant, + protocol.MethodConfigPermissionsRevoke, + + // Agents — create/update/delete and link mutations. protocol.MethodAgentsCreate, protocol.MethodAgentsUpdate, protocol.MethodAgentsDelete, - protocol.MethodAgentsLinksList, protocol.MethodAgentsLinksCreate, protocol.MethodAgentsLinksUpdate, protocol.MethodAgentsLinksDelete, + + // Channels. protocol.MethodChannelsToggle, + protocol.MethodChannelInstancesCreate, + protocol.MethodChannelInstancesUpdate, + protocol.MethodChannelInstancesDelete, + + // Pairing management (approve/revoke/list/deny require admin). protocol.MethodPairingApprove, + protocol.MethodPairingDeny, + protocol.MethodPairingList, protocol.MethodPairingRevoke, - protocol.MethodTeamsList, + + // Teams — create/delete/update/member management. protocol.MethodTeamsCreate, - protocol.MethodTeamsGet, protocol.MethodTeamsDelete, - protocol.MethodTeamsTaskList, - protocol.MethodTeamsTaskGet, - protocol.MethodTeamsTaskComments, - protocol.MethodTeamsTaskEvents, + protocol.MethodTeamsUpdate, + protocol.MethodTeamsMembersAdd, + protocol.MethodTeamsMembersRemove, + protocol.MethodTeamsTaskDelete, + protocol.MethodTeamsTaskDeleteBulk, + + // Tenants — write paths. + "tenants.create", + "tenants.update", + "tenants.users.add", + "tenants.users.remove", + + // API keys expose secret material — gate list + mutations as admin. protocol.MethodAPIKeysList, protocol.MethodAPIKeysCreate, protocol.MethodAPIKeysRevoke, + + // Skills (can rewrite agent behavior). protocol.MethodSkillsUpdate, + + // Heartbeat — any write/test path (closes CVE #866 step 2 + step 4). + protocol.MethodHeartbeatSet, + protocol.MethodHeartbeatToggle, + protocol.MethodHeartbeatTest, + protocol.MethodHeartbeatChecklistSet, + + // Live server logs — data exfiltration risk (closes CVE #866 step 3). + protocol.MethodLogsTail, + + // Hooks mutations (the handler middleware also enforces this). + protocol.MethodHooksCreate, + protocol.MethodHooksUpdate, + protocol.MethodHooksDelete, + protocol.MethodHooksToggle, + + // Voice catalogue refresh touches provider credentials. + protocol.MethodVoicesRefresh, + + // TTS config mutations touch provider credentials / global state. + protocol.MethodTTSEnable, + protocol.MethodTTSDisable, + protocol.MethodTTSSetProvider, } return slices.Contains(adminMethods, method) } func isWriteMethod(method string) bool { - writePrefixes := []string{ + writeExact := []string{ protocol.MethodChatSend, protocol.MethodChatAbort, + protocol.MethodChatInject, protocol.MethodSessionsDelete, protocol.MethodSessionsReset, protocol.MethodSessionsPatch, @@ -203,23 +296,126 @@ func isWriteMethod(method string) bool { protocol.MethodCronUpdate, protocol.MethodCronDelete, protocol.MethodCronToggle, - "pairing.", - "device.pair.", - "approvals.", - "exec.approval.", + protocol.MethodCronRun, protocol.MethodSend, + protocol.MethodAgentsFileSet, protocol.MethodTeamsTaskApprove, protocol.MethodTeamsTaskReject, protocol.MethodTeamsTaskComment, protocol.MethodTeamsTaskCreate, protocol.MethodTeamsTaskAssign, + protocol.MethodTeamsWorkspaceDelete, + protocol.MethodHooksTest, + protocol.MethodPairingRequest, + protocol.MethodApprovalsApprove, + protocol.MethodApprovalsDeny, + + // TTS synthesis — invokes provider API (quota/credentials). + protocol.MethodTTSConvert, + + // Browser automation — performs side-effecting actions. + protocol.MethodBrowserAct, + + // Channel pairing starts (QR scan flows). + protocol.MethodZaloPersonalQRStart, + protocol.MethodWhatsAppQRStart, } - for _, prefix := range writePrefixes { - if strings.HasPrefix(method, prefix) { - return true - } + return slices.Contains(writeExact, method) +} + +// isReadMethod is the explicit viewer-allowlist for read-only RPCs. Keeping +// this as a closed list (rather than "everything else") is what turns the +// policy into fail-closed. New read RPCs must be added here explicitly. +func isReadMethod(method string) bool { + readMethods := []string{ + // Agent identity / wait + protocol.MethodAgent, + protocol.MethodAgentWait, + protocol.MethodAgentIdentityGet, + + // Chat read + protocol.MethodChatHistory, + protocol.MethodChatSessionStatus, + + // Agents read + protocol.MethodAgentsList, + protocol.MethodAgentsFileList, + protocol.MethodAgentsFileGet, + protocol.MethodAgentsLinksList, + + // Sessions read + protocol.MethodSessionsList, + protocol.MethodSessionsPreview, + + // Skills read + protocol.MethodSkillsList, + protocol.MethodSkillsGet, + + // Cron read + protocol.MethodCronList, + protocol.MethodCronStatus, + protocol.MethodCronRuns, + + // Channels read + protocol.MethodChannelsList, + protocol.MethodChannelsStatus, + protocol.MethodChannelInstancesList, + protocol.MethodChannelInstancesGet, + + // Usage / quota + protocol.MethodUsageGet, + protocol.MethodUsageSummary, + protocol.MethodQuotaUsage, + + // Heartbeat read + protocol.MethodHeartbeatGet, + protocol.MethodHeartbeatLogs, + protocol.MethodHeartbeatChecklistGet, + protocol.MethodHeartbeatTargets, + + // Voices + protocol.MethodVoicesList, + + // Tenants read + "tenants.list", + "tenants.get", + "tenants.users.list", + "tenants.mine", + + // Teams read + protocol.MethodTeamsList, + protocol.MethodTeamsGet, + protocol.MethodTeamsTaskList, + protocol.MethodTeamsTaskGet, + protocol.MethodTeamsTaskGetLight, + protocol.MethodTeamsTaskComments, + protocol.MethodTeamsTaskEvents, + protocol.MethodTeamsTaskActiveBySession, + protocol.MethodTeamsWorkspaceList, + protocol.MethodTeamsWorkspaceRead, + protocol.MethodTeamsEventsList, + protocol.MethodTeamsKnownUsers, + protocol.MethodTeamsScopes, + + // Hooks read + protocol.MethodHooksList, + protocol.MethodHooksHistory, + + // Approvals read-only listing + protocol.MethodApprovalsList, + + // TTS read-only (status/providers listing) + protocol.MethodTTSStatus, + protocol.MethodTTSProviders, + + // Browser observation (no side effects) + protocol.MethodBrowserSnapshot, + protocol.MethodBrowserScreenshot, + + // Zalo personal contacts listing + protocol.MethodZaloPersonalContacts, } - return false + return slices.Contains(readMethods, method) } // HasMinRole checks if the given role meets the minimum required level. diff --git a/internal/permissions/policy_test.go b/internal/permissions/policy_test.go index 783ae7d1..df5d1e45 100644 --- a/internal/permissions/policy_test.go +++ b/internal/permissions/policy_test.go @@ -1,6 +1,12 @@ package permissions import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "runtime" + "strings" "testing" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -147,11 +153,71 @@ func TestCanAccess_ReadMethods_AnyRole(t *testing.T) { } } -func TestCanAccess_UnknownMethod_DefaultsToViewer(t *testing.T) { +// TestCanAccess_UnknownMethod_DeniedForAll locks in the fail-closed behavior +// introduced by issue #866: any method not present in the public / admin / +// write / read allowlists MUST be denied for every role including owner. +// This is the inverse of the previous default-permit behavior. +func TestCanAccess_UnknownMethod_DeniedForAll(t *testing.T) { pe := NewPolicyEngine(nil) - // Unknown method defaults to viewer role requirement - if !pe.CanAccess(RoleViewer, "totally.unknown.method") { - t.Fatal("viewer should access unknown method (defaults to viewer)") + unknown := "totally.unknown.method" + for _, role := range []Role{RoleViewer, RoleOperator, RoleAdmin, RoleOwner} { + if pe.CanAccess(role, unknown) { + t.Fatalf("%s must NOT access unclassified method %q (fail-closed)", role, unknown) + } + } + if MethodRole(unknown) != RoleNone { + t.Fatalf("MethodRole(%q) = %q, want RoleNone", unknown, MethodRole(unknown)) + } +} + +// TestCanAccess_CVE866_HeartbeatAndLogs asserts that the three RPCs exploited +// in the issue-#866 chain now require admin. Viewer/operator must be denied. +func TestCanAccess_CVE866_HeartbeatAndLogs(t *testing.T) { + pe := NewPolicyEngine(nil) + adminOnly := []string{ + protocol.MethodHeartbeatSet, + protocol.MethodHeartbeatChecklistSet, + protocol.MethodLogsTail, + protocol.MethodHeartbeatToggle, + protocol.MethodHeartbeatTest, + } + for _, method := range adminOnly { + t.Run(method, func(t *testing.T) { + if pe.CanAccess(RoleViewer, method) { + t.Fatalf("viewer must NOT access %s (CVE #866)", method) + } + if pe.CanAccess(RoleOperator, method) { + t.Fatalf("operator must NOT access %s (CVE #866)", method) + } + if !pe.CanAccess(RoleAdmin, method) { + t.Fatalf("admin should access %s", method) + } + if !pe.CanAccess(RoleOwner, method) { + t.Fatalf("owner should access %s", method) + } + }) + } +} + +// TestCanAccess_PublicMethods ensures pre-auth RPCs remain reachable by every +// role (including the zero-value RoleNone placeholder that pre-connect clients +// hold) — otherwise legitimate clients cannot even complete the handshake. +func TestCanAccess_PublicMethods(t *testing.T) { + pe := NewPolicyEngine(nil) + public := []string{ + protocol.MethodConnect, + protocol.MethodHealth, + protocol.MethodStatus, + protocol.MethodBrowserPairingStatus, + } + for _, method := range public { + t.Run(method, func(t *testing.T) { + for _, role := range []Role{RoleViewer, RoleOperator, RoleAdmin, RoleOwner} { + if !pe.CanAccess(role, method) { + t.Fatalf("%s must access public method %s", role, method) + } + } + }) } } @@ -241,6 +307,82 @@ func TestValidScope(t *testing.T) { } } +// --- MethodApprovalsList: locks in fix for writePrefixes shadowing bug. +// Before the fix, the "exec.approval." prefix in isWriteMethod's writePrefixes +// short-circuited the public→admin→write→read ordering in MethodRole, +// wrongly classifying exec.approval.list as RoleOperator. exec.approval.list +// is an explicit entry in isReadMethod and must resolve to RoleViewer. + +func TestMethodRole_ApprovalsList_IsViewer(t *testing.T) { + if got := MethodRole(protocol.MethodApprovalsList); got != RoleViewer { + t.Fatalf("exec.approval.list must be RoleViewer (listed in isReadMethod); got %q", got) + } + if got := MethodRole(protocol.MethodApprovalsApprove); got != RoleOperator { + t.Fatalf("exec.approval.approve must be RoleOperator; got %q", got) + } + if got := MethodRole(protocol.MethodApprovalsDeny); got != RoleOperator { + t.Fatalf("exec.approval.deny must be RoleOperator; got %q", got) + } +} + +// --- Drift coverage: parses pkg/protocol/methods.go at test time, enumerates +// every const Method* = "...", and asserts none resolve to RoleNone. New RPCs +// added without a matching allowlist entry will be caught here before shipping +// as fail-closed rejections in production. + +func TestMethodRole_DriftCoverage_AllProtocolMethodsClassified(t *testing.T) { + _, thisFile, _, _ := runtime.Caller(0) + methodsGoPath := filepath.Join(filepath.Dir(thisFile), "..", "..", "pkg", "protocol", "methods.go") + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, methodsGoPath, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", methodsGoPath, err) + } + + var methods []string + for _, decl := range f.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.CONST { + continue + } + for _, spec := range gd.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for i, name := range vs.Names { + if !strings.HasPrefix(name.Name, "Method") { + continue + } + if i >= len(vs.Values) { + continue + } + lit, ok := vs.Values[i].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + continue + } + methods = append(methods, strings.Trim(lit.Value, `"`)) + } + } + } + + if len(methods) < 100 { + t.Fatalf("expected 100+ Method* constants, parser collected %d — methods.go layout may have changed", len(methods)) + } + + var unclassified []string + for _, m := range methods { + if MethodRole(m) == RoleNone { + unclassified = append(unclassified, m) + } + } + if len(unclassified) > 0 { + t.Fatalf("RBAC drift — %d protocol method(s) resolve to RoleNone:\n %s\n\nAdd each to isPublicMethod, isAdminMethod, isWriteMethod, or isReadMethod in policy.go.", + len(unclassified), strings.Join(unclassified, "\n ")) + } +} + // --- MethodScopes: verify pairing/approvals special routes --- func TestMethodScopes_PairingMethod(t *testing.T) {