fix(security): close auth bypass + default-permit RBAC (issue #866) (#950)

Fixes a 3-vuln chain enabling unauthenticated/underprivileged access to
admin surfaces:

1. Auth bypass (router.go): invalid tokens silently fell through to
   anonymous role. Now reject on token presence with invalid/expired.
2. Default-permit RBAC (policy.go): unknown methods returned a writable
   role. Now fail-closed: unknown -> RoleNone.
3. Missing admin classifications: several sensitive methods weren't in
   any allowlist and slipped through default-permit. Now explicitly
   classified (admin/write/read).

Additional hardening:
- Remove isWriteMethod prefix fallbacks; use exact allowlist only to
  prevent silent scope creep when new methods are added.
- Classify Phase-3 methods (TTS, browser, zalo, whatsapp) into
  admin/write/read per sensitivity.
- Gate empty MethodRole in router dispatcher.

Tests:
- TestMethodRole_ApprovalsList_IsViewer: asserts exec.approval.list is
  viewer and approve/deny are operator.
- TestMethodRole_DriftCoverage_AllProtocolMethodsClassified: AST-parses
  pkg/protocol/methods.go at test time; asserts every Method* constant
  resolves to a non-None role. Permanent drift guard for future adds.

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) {

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.

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) {