mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-12 22:14:37 +00:00
viettranx
6d7473b56f
Phase 0b of tenant tool config refactor. Closes 3 privilege-escalation
vulnerabilities in the same bug class as commit b419f352 (Phase 1
config.* hotfix):
- CRITICAL: PUT /v1/tools/builtin/{name} — non-master tenant admin
could overwrite global builtin_tools.settings, corrupting tool
defaults for every tenant.
- CRITICAL: POST /v1/packages/install|uninstall — non-master tenant
admin could run pip/npm/apk server-wide. Supply-chain vector.
- HIGH: POST /v1/api-keys/{id}/revoke (HTTP + WS) — tenant admin
could revoke NULL-tenant system keys because store SQL matches
(tenant_id = \$N OR tenant_id IS NULL).
Implementation:
- Export store.IsMasterScope as the single predicate; rewire Phase 1
config.* middleware to delegate (no behaviour change).
- Add http.requireMasterScope helper symmetric to requireTenantAdmin.
- Guard handleUpdate (builtin_tools) and handleInstall/handleUninstall
(packages) with master-scope check before any mutation or shell exec.
- Fix api_keys.Revoke at the handler layer: fetch key via new
APIKeyStore.Get, verify key.TenantID matches caller tenant for
non-owner callers. Applies to both HTTP and WS paths.
- Harden WS router to inject role into ctx so store.IsOwnerRole works
from WS handlers (closes a latent drift between the HTTP and WS
layers that broke the initial WS api_keys.revoke fix).
- Drop unused APIKeyStore.Delete (YAGNI + removes dormant vuln with
the same tenant_id IS NULL arm).
- Emit security.tenant_scope_violation and security.api_key_revoke_
forbidden slog events on every rejection for future SIEM alerting.
- New MsgMasterScopeRequired i18n key + en/vi/zh catalogs.
Tests cover the guard predicate, all 3 HTTP endpoints, and the full
WS api_keys.revoke matrix (cross-tenant deny, system-key deny for
tenant admins, system owner bypass, own-tenant happy path). 14 other
admin-gated write endpoints verified safe by static audit.
88 lines
3.1 KiB
Go
88 lines
3.1 KiB
Go
package http
|
|
|
|
import (
|
|
"log/slog"
|
|
"net/http"
|
|
|
|
"github.com/google/uuid"
|
|
|
|
"github.com/nextlevelbuilder/goclaw/internal/i18n"
|
|
"github.com/nextlevelbuilder/goclaw/internal/store"
|
|
)
|
|
|
|
// requireTenantAdmin verifies the caller has owner or admin role within the
|
|
// specified tenant. System-wide owners (IsOwnerRole) bypass the check.
|
|
// Returns true if authorized, false if an error response was written.
|
|
//
|
|
// WARNING: the system-owner bypass does NOT guarantee a non-nil tenant ID
|
|
// downstream. A system owner can reach tenant-config handlers with
|
|
// tid == uuid.Nil. Handlers that branch on tenant ID (e.g. emitting
|
|
// tenant-scoped cache invalidate events) must therefore guard against
|
|
// uuid.Nil explicitly — don't rely on this helper to enforce it.
|
|
func requireTenantAdmin(w http.ResponseWriter, r *http.Request, ts store.TenantStore) bool {
|
|
ctx := r.Context()
|
|
|
|
// System-wide owner bypasses tenant membership check.
|
|
if store.IsOwnerRole(ctx) {
|
|
return true
|
|
}
|
|
|
|
if ts == nil {
|
|
writeJSON(w, http.StatusNotImplemented, map[string]string{"error": "tenant store not available"})
|
|
return false
|
|
}
|
|
|
|
tid := store.TenantIDFromContext(ctx)
|
|
if tid == uuid.Nil {
|
|
locale := store.LocaleFromContext(ctx)
|
|
writeJSON(w, http.StatusBadRequest, map[string]string{
|
|
"error": i18n.T(locale, i18n.MsgPermissionDenied, "tenant config"),
|
|
})
|
|
return false
|
|
}
|
|
|
|
userID := store.UserIDFromContext(ctx)
|
|
// GetUserRole returns ("", nil) when the user has no membership in this tenant,
|
|
// which correctly falls through to the role check below (denied).
|
|
role, err := ts.GetUserRole(ctx, tid, userID)
|
|
if err != nil || (role != store.TenantRoleOwner && role != store.TenantRoleAdmin) {
|
|
locale := store.LocaleFromContext(ctx)
|
|
writeJSON(w, http.StatusForbidden, map[string]string{
|
|
"error": i18n.T(locale, i18n.MsgPermissionDenied, "tenant config"),
|
|
})
|
|
return false
|
|
}
|
|
return true
|
|
}
|
|
|
|
// requireMasterScope guards endpoints that write to global (non-tenant-scoped)
|
|
// tables or execute server-wide side effects (shell, filesystem). Rejects
|
|
// callers whose ctx is scoped to a non-master tenant even if they hold
|
|
// RoleAdmin in that tenant. System owners bypass.
|
|
//
|
|
// Symmetric counterpart to requireTenantAdmin: one guards tenant-scoped
|
|
// writes, the other guards global writes. Use on any PUT/POST/DELETE handler
|
|
// touching builtin_tools, disk config, package management, or similar global
|
|
// state. Shares the predicate with WS layer via store.IsMasterScope so rules
|
|
// cannot drift between transports.
|
|
//
|
|
// Returns true on allow, false on deny (in which case a 403 response has
|
|
// already been written). Emits security.tenant_scope_violation slog on deny.
|
|
func requireMasterScope(w http.ResponseWriter, r *http.Request) bool {
|
|
ctx := r.Context()
|
|
if store.IsMasterScope(ctx) {
|
|
return true
|
|
}
|
|
slog.Warn("security.tenant_scope_violation",
|
|
"path", r.URL.Path,
|
|
"method", r.Method,
|
|
"tenant_id", store.TenantIDFromContext(ctx),
|
|
"user_id", store.UserIDFromContext(ctx),
|
|
)
|
|
locale := store.LocaleFromContext(ctx)
|
|
writeJSON(w, http.StatusForbidden, map[string]string{
|
|
"error": i18n.T(locale, i18n.MsgMasterScopeRequired),
|
|
})
|
|
return false
|
|
}
|