mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-07-12 19:04:49 +00:00
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.
228 lines
5.6 KiB
Go
228 lines
5.6 KiB
Go
package http
|
|
|
|
import (
|
|
"context"
|
|
"sync"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/google/uuid"
|
|
|
|
"github.com/nextlevelbuilder/goclaw/internal/permissions"
|
|
"github.com/nextlevelbuilder/goclaw/internal/store"
|
|
)
|
|
|
|
// mockAPIKeyStore is a minimal mock for testing the cache layer.
|
|
type mockAPIKeyStore struct {
|
|
mu sync.Mutex
|
|
keys map[string]*store.APIKeyData // hash → key
|
|
byID map[uuid.UUID]*store.APIKeyData
|
|
calls int // GetByHash call count
|
|
touchedID uuid.UUID // last TouchLastUsed ID
|
|
}
|
|
|
|
func newMockAPIKeyStore() *mockAPIKeyStore {
|
|
return &mockAPIKeyStore{
|
|
keys: make(map[string]*store.APIKeyData),
|
|
byID: make(map[uuid.UUID]*store.APIKeyData),
|
|
}
|
|
}
|
|
|
|
func (m *mockAPIKeyStore) Get(_ context.Context, id uuid.UUID) (*store.APIKeyData, error) {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
if k, ok := m.byID[id]; ok {
|
|
return k, nil
|
|
}
|
|
return nil, nil
|
|
}
|
|
|
|
func (m *mockAPIKeyStore) GetByHash(_ context.Context, hash string) (*store.APIKeyData, error) {
|
|
m.mu.Lock()
|
|
m.calls++
|
|
m.mu.Unlock()
|
|
if k, ok := m.keys[hash]; ok {
|
|
return k, nil
|
|
}
|
|
return nil, nil
|
|
}
|
|
|
|
func (m *mockAPIKeyStore) TouchLastUsed(_ context.Context, id uuid.UUID) error {
|
|
m.mu.Lock()
|
|
m.touchedID = id
|
|
m.mu.Unlock()
|
|
return nil
|
|
}
|
|
|
|
func (m *mockAPIKeyStore) Create(_ context.Context, _ *store.APIKeyData) error { return nil }
|
|
func (m *mockAPIKeyStore) List(_ context.Context, _ string) ([]store.APIKeyData, error) {
|
|
return nil, nil
|
|
}
|
|
func (m *mockAPIKeyStore) Revoke(_ context.Context, _ uuid.UUID, _ string) error { return nil }
|
|
|
|
func (m *mockAPIKeyStore) getCalls() int {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
return m.calls
|
|
}
|
|
|
|
func TestCacheMissFetchesFromStore(t *testing.T) {
|
|
ms := newMockAPIKeyStore()
|
|
keyID := uuid.New()
|
|
ms.keys["hash123"] = &store.APIKeyData{
|
|
ID: keyID,
|
|
Scopes: []string{"operator.read"},
|
|
}
|
|
|
|
c := newAPIKeyCache(ms, 5*time.Minute)
|
|
key, role := c.getOrFetch(context.Background(), "hash123")
|
|
|
|
if key == nil {
|
|
t.Fatal("expected key, got nil")
|
|
}
|
|
if key.ID != keyID {
|
|
t.Errorf("key ID = %v, want %v", key.ID, keyID)
|
|
}
|
|
if role != permissions.RoleViewer {
|
|
t.Errorf("role = %v, want %v", role, permissions.RoleViewer)
|
|
}
|
|
if ms.getCalls() != 1 {
|
|
t.Errorf("store calls = %d, want 1", ms.getCalls())
|
|
}
|
|
}
|
|
|
|
func TestCacheHitReturnsWithoutStoreCall(t *testing.T) {
|
|
ms := newMockAPIKeyStore()
|
|
keyID := uuid.New()
|
|
ms.keys["hash456"] = &store.APIKeyData{
|
|
ID: keyID,
|
|
Scopes: []string{"operator.admin"},
|
|
}
|
|
|
|
c := newAPIKeyCache(ms, 5*time.Minute)
|
|
|
|
// First call: cache miss → store fetch
|
|
c.getOrFetch(context.Background(), "hash456")
|
|
// Second call: cache hit → no store fetch
|
|
key, role := c.getOrFetch(context.Background(), "hash456")
|
|
|
|
if key == nil {
|
|
t.Fatal("expected key on cache hit, got nil")
|
|
}
|
|
if role != permissions.RoleAdmin {
|
|
t.Errorf("role = %v, want %v", role, permissions.RoleAdmin)
|
|
}
|
|
if ms.getCalls() != 1 {
|
|
t.Errorf("store calls = %d, want 1 (cache hit should not call store)", ms.getCalls())
|
|
}
|
|
}
|
|
|
|
func TestCacheTTLExpiry(t *testing.T) {
|
|
ms := newMockAPIKeyStore()
|
|
ms.keys["hash789"] = &store.APIKeyData{
|
|
ID: uuid.New(),
|
|
Scopes: []string{"operator.write"},
|
|
}
|
|
|
|
c := newAPIKeyCache(ms, 10*time.Millisecond) // very short TTL
|
|
|
|
// First fetch
|
|
c.getOrFetch(context.Background(), "hash789")
|
|
if ms.getCalls() != 1 {
|
|
t.Fatalf("initial calls = %d, want 1", ms.getCalls())
|
|
}
|
|
|
|
// Wait for TTL to expire
|
|
time.Sleep(20 * time.Millisecond)
|
|
|
|
// Should re-fetch from store
|
|
key, _ := c.getOrFetch(context.Background(), "hash789")
|
|
if key == nil {
|
|
t.Fatal("expected key after TTL expiry")
|
|
}
|
|
if ms.getCalls() != 2 {
|
|
t.Errorf("store calls after TTL = %d, want 2", ms.getCalls())
|
|
}
|
|
}
|
|
|
|
func TestCacheInvalidateAll(t *testing.T) {
|
|
ms := newMockAPIKeyStore()
|
|
ms.keys["hashA"] = &store.APIKeyData{
|
|
ID: uuid.New(),
|
|
Scopes: []string{"operator.read"},
|
|
}
|
|
|
|
c := newAPIKeyCache(ms, 5*time.Minute)
|
|
|
|
// Populate cache
|
|
c.getOrFetch(context.Background(), "hashA")
|
|
if ms.getCalls() != 1 {
|
|
t.Fatalf("initial calls = %d, want 1", ms.getCalls())
|
|
}
|
|
|
|
// Invalidate
|
|
c.invalidateAll()
|
|
|
|
// Should fetch again from store
|
|
c.getOrFetch(context.Background(), "hashA")
|
|
if ms.getCalls() != 2 {
|
|
t.Errorf("store calls after invalidate = %d, want 2", ms.getCalls())
|
|
}
|
|
}
|
|
|
|
func TestCacheNegativeCache(t *testing.T) {
|
|
ms := newMockAPIKeyStore()
|
|
// No keys in store
|
|
|
|
c := newAPIKeyCache(ms, 5*time.Minute)
|
|
|
|
// First lookup: cache miss → store returns nil → negative cache
|
|
key1, _ := c.getOrFetch(context.Background(), "unknown")
|
|
if key1 != nil {
|
|
t.Fatal("expected nil key for unknown hash")
|
|
}
|
|
if ms.getCalls() != 1 {
|
|
t.Fatalf("initial calls = %d, want 1", ms.getCalls())
|
|
}
|
|
|
|
// Second lookup: negative cache hit → no store call
|
|
key2, _ := c.getOrFetch(context.Background(), "unknown")
|
|
if key2 != nil {
|
|
t.Fatal("expected nil key on negative cache hit")
|
|
}
|
|
if ms.getCalls() != 1 {
|
|
t.Errorf("store calls after negative cache = %d, want 1", ms.getCalls())
|
|
}
|
|
}
|
|
|
|
func TestCacheConcurrentAccess(t *testing.T) {
|
|
ms := newMockAPIKeyStore()
|
|
ms.keys["concurrent"] = &store.APIKeyData{
|
|
ID: uuid.New(),
|
|
Scopes: []string{"operator.admin"},
|
|
}
|
|
|
|
c := newAPIKeyCache(ms, 5*time.Minute)
|
|
|
|
var wg sync.WaitGroup
|
|
for range 50 {
|
|
wg.Go(func() {
|
|
key, role := c.getOrFetch(context.Background(), "concurrent")
|
|
if key == nil {
|
|
t.Error("expected key, got nil")
|
|
}
|
|
if role != permissions.RoleAdmin {
|
|
t.Errorf("role = %v, want admin", role)
|
|
}
|
|
})
|
|
}
|
|
wg.Wait()
|
|
|
|
// Store should have been called at least once, but not 50 times
|
|
// (cache may have been populated after the first call)
|
|
calls := ms.getCalls()
|
|
if calls == 0 || calls > 50 {
|
|
t.Errorf("store calls = %d, want 1..50", calls)
|
|
}
|
|
}
|