diff --git a/internal/gateway/client_testing.go b/internal/gateway/client_testing.go new file mode 100644 index 00000000..b7bb400f --- /dev/null +++ b/internal/gateway/client_testing.go @@ -0,0 +1,23 @@ +package gateway + +import ( + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/permissions" +) + +// NewTestClient returns a minimally-wired Client for unit tests in other +// packages. Role + tenant are set directly because the underlying fields are +// unexported. SendResponse is safe because the send channel is nil — the +// writer hits the default branch of the select and drops the frame silently. +// +// Not for production use. Any non-test caller should use NewClient instead. +func NewTestClient(role permissions.Role, tenantID uuid.UUID, userID string) *Client { + return &Client{ + id: uuid.NewString(), + authenticated: true, + role: role, + userID: userID, + tenantID: tenantID, + } +} diff --git a/internal/gateway/methods/api_keys.go b/internal/gateway/methods/api_keys.go index a3bf1f29..25d38370 100644 --- a/internal/gateway/methods/api_keys.go +++ b/internal/gateway/methods/api_keys.go @@ -163,6 +163,14 @@ func (m *APIKeysMethods) handleCreate(ctx context.Context, client *gateway.Clien })) } +// handleRevoke revokes an API key after verifying ownership. +// +// Phase 0b hotfix: store-layer Revoke SQL matches on +// `tenant_id = $N OR tenant_id IS NULL`, which previously allowed any +// tenant admin to revoke system-level (NULL-tenant) API keys. The fix +// pre-fetches the key and enforces strict tenant match for non-owner +// callers. Non-admin callers continue to use the ownerID filter path +// (unchanged behaviour for personal keys). func (m *APIKeysMethods) handleRevoke(ctx context.Context, client *gateway.Client, req *protocol.RequestFrame) { locale := store.LocaleFromContext(ctx) @@ -182,12 +190,40 @@ func (m *APIKeysMethods) handleRevoke(ctx context.Context, client *gateway.Clien return } - // Non-admin callers can only revoke their own keys. + // Non-admin callers can only revoke their own keys — personal-key path, + // ownerID filter enforced by the store layer. ownerID := "" if !permissions.HasMinRole(client.Role(), permissions.RoleAdmin) { ownerID = client.UserID() } + // Admin path: verify the target key belongs to the caller's tenant (or + // caller is a system owner) before revoking. Personal-key path skips + // this because the ownerID filter already scopes to the caller. + // + // NOTE: Use client.IsOwner() — NOT store.IsOwnerRole(ctx). The WS router + // does not inject role into ctx (see router.go handleRequest), so the + // ctx-based helper is dead here. Client carries the authoritative role + // from connect. + if ownerID == "" && !client.IsOwner() { + key, gerr := m.apiKeys.Get(ctx, id) + if gerr != nil || key == nil { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrNotFound, i18n.T(locale, i18n.MsgNotFound, "API key", params.ID))) + return + } + callerTID := store.TenantIDFromContext(ctx) + if key.TenantID == uuid.Nil || key.TenantID != callerTID { + slog.Warn("security.api_key_revoke_forbidden", + "key_id", params.ID, + "caller_tenant", callerTID, + "key_tenant", key.TenantID, + "user_id", client.UserID(), + ) + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrUnauthorized, i18n.T(locale, i18n.MsgPermissionDenied, "API key"))) + return + } + } + if err := m.apiKeys.Revoke(ctx, id, ownerID); err != nil { slog.Error("api_keys.revoke failed", "error", err, "id", params.ID) client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrNotFound, i18n.T(locale, i18n.MsgNotFound, "API key", params.ID))) diff --git a/internal/gateway/methods/api_keys_tenant_guard_test.go b/internal/gateway/methods/api_keys_tenant_guard_test.go new file mode 100644 index 00000000..74b2a2d0 --- /dev/null +++ b/internal/gateway/methods/api_keys_tenant_guard_test.go @@ -0,0 +1,236 @@ +package methods + +import ( + "context" + "encoding/json" + "errors" + "slices" + "sync" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/gateway" + "github.com/nextlevelbuilder/goclaw/internal/permissions" + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" +) + +// Phase 0b review regression coverage: WS api_keys.revoke. +// +// The HTTP fix in internal/http/api_keys.go has its own test file, but the +// original Phase 0b WS fix (internal/gateway/methods/api_keys.go) had zero +// test coverage — and a Major bug slipped past: store.IsOwnerRole(ctx) +// always returns false in WS handlers because the router does not inject +// role into ctx. The bug was caught by code review, not tests. These tests +// lock the fix in place and cover the four critical paths. + +// ---- minimal stub store ---- + +type stubAPIKeyStore struct { + mu sync.Mutex + byID map[uuid.UUID]*store.APIKeyData + revokedCalls []uuid.UUID +} + +func newStubAPIKeyStore() *stubAPIKeyStore { + return &stubAPIKeyStore{byID: make(map[uuid.UUID]*store.APIKeyData)} +} + +func (s *stubAPIKeyStore) Create(_ context.Context, k *store.APIKeyData) error { + s.mu.Lock() + defer s.mu.Unlock() + s.byID[k.ID] = k + return nil +} + +func (s *stubAPIKeyStore) Get(_ context.Context, id uuid.UUID) (*store.APIKeyData, error) { + s.mu.Lock() + defer s.mu.Unlock() + if k, ok := s.byID[id]; ok { + return k, nil + } + return nil, errors.New("not found") +} + +func (s *stubAPIKeyStore) GetByHash(_ context.Context, _ string) (*store.APIKeyData, error) { + return nil, nil +} + +func (s *stubAPIKeyStore) List(_ context.Context, _ string) ([]store.APIKeyData, error) { + return nil, nil +} + +func (s *stubAPIKeyStore) Revoke(_ context.Context, id uuid.UUID, _ string) error { + s.mu.Lock() + defer s.mu.Unlock() + s.revokedCalls = append(s.revokedCalls, id) + if k, ok := s.byID[id]; ok { + k.Revoked = true + } + return nil +} + +func (s *stubAPIKeyStore) TouchLastUsed(_ context.Context, _ uuid.UUID) error { return nil } + +func (s *stubAPIKeyStore) wasRevoked(id uuid.UUID) bool { + s.mu.Lock() + defer s.mu.Unlock() + return slices.Contains(s.revokedCalls, id) +} + +// wsCallCtx mirrors what router.handleRequest injects into ctx for method +// handlers: locale, tenantID (from client.TenantID()), and role (added +// by Phase 0b hardening). Tests bypass the router, so we reproduce the +// minimum set the handler reads. +func wsCallCtx(client *gateway.Client) context.Context { + ctx := context.Background() + if tid := client.TenantID(); tid != uuid.Nil { + ctx = store.WithTenantID(ctx, tid) + } + if role := client.Role(); role != "" { + ctx = store.WithRole(ctx, string(role)) + } + return ctx +} + +// ---- harness ---- + +func buildRevokeRequest(t *testing.T, keyID uuid.UUID) *protocol.RequestFrame { + t.Helper() + raw, err := json.Marshal(map[string]string{"id": keyID.String()}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return &protocol.RequestFrame{ + Type: protocol.FrameTypeRequest, + ID: "revoke-req-1", + Method: protocol.MethodAPIKeysRevoke, + Params: raw, + } +} + +// ---- tests ---- + +// TestWSRevoke_TenantAdmin_CrossTenantKey_Denied is the core HIGH finding: +// a tenant admin attempting to revoke a key owned by another tenant must +// be rejected, and Revoke must not be called on the store. +func TestWSRevoke_TenantAdmin_CrossTenantKey_Denied(t *testing.T) { + callerTID := uuid.MustParse("11111111-1111-1111-1111-111111111111") + keyOwnerTID := uuid.MustParse("22222222-2222-2222-2222-222222222222") + keyID := uuid.New() + + stub := newStubAPIKeyStore() + _ = stub.Create(context.Background(), &store.APIKeyData{ + ID: keyID, + Name: "other-tenant-key", + TenantID: keyOwnerTID, + }) + + m := &APIKeysMethods{apiKeys: stub} + client := gateway.NewTestClient(permissions.RoleAdmin, callerTID, "user-1") + + m.handleRevoke(wsCallCtx(client), client, buildRevokeRequest(t, keyID)) + + if stub.wasRevoked(keyID) { + t.Fatalf("cross-tenant key should NOT have been revoked") + } +} + +// TestWSRevoke_TenantAdmin_SystemKey_Denied is the second HIGH path: the +// NULL-tenant (system) key must not be revocable by a tenant admin. +func TestWSRevoke_TenantAdmin_SystemKey_Denied(t *testing.T) { + callerTID := uuid.MustParse("33333333-3333-3333-3333-333333333333") + keyID := uuid.New() + + stub := newStubAPIKeyStore() + _ = stub.Create(context.Background(), &store.APIKeyData{ + ID: keyID, + Name: "system-ci-key", + TenantID: uuid.Nil, // NULL-tenant system key + }) + + m := &APIKeysMethods{apiKeys: stub} + client := gateway.NewTestClient(permissions.RoleAdmin, callerTID, "user-1") + + m.handleRevoke(wsCallCtx(client), client, buildRevokeRequest(t, keyID)) + + if stub.wasRevoked(keyID) { + t.Fatalf("system (NULL-tenant) key should NOT have been revoked by tenant admin") + } +} + +// TestWSRevoke_SystemOwner_SystemKey_Allowed is the regression caught by +// code review: the original Phase 0b fix used store.IsOwnerRole(ctx), +// which returns false in WS because the router does not inject role into +// ctx. This test exercises the now-correct client.IsOwner() path. +func TestWSRevoke_SystemOwner_SystemKey_Allowed(t *testing.T) { + keyID := uuid.New() + + stub := newStubAPIKeyStore() + _ = stub.Create(context.Background(), &store.APIKeyData{ + ID: keyID, + Name: "system-ci-key", + TenantID: uuid.Nil, + }) + + m := &APIKeysMethods{apiKeys: stub} + // Owner client — narrowed-to-master tenant scope is the default for + // owner logins per router.applyTenantScope. + client := gateway.NewTestClient(permissions.RoleOwner, store.MasterTenantID, "owner-1") + + m.handleRevoke(wsCallCtx(client), client, buildRevokeRequest(t, keyID)) + + if !stub.wasRevoked(keyID) { + t.Fatalf("system owner must be allowed to revoke NULL-tenant system key") + } +} + +// TestWSRevoke_TenantAdmin_OwnTenantKey_Allowed is the happy path: admin +// revoking a key inside their own tenant — no regression for legitimate +// scope-matching revocations. +func TestWSRevoke_TenantAdmin_OwnTenantKey_Allowed(t *testing.T) { + tid := uuid.MustParse("44444444-4444-4444-4444-444444444444") + keyID := uuid.New() + + stub := newStubAPIKeyStore() + _ = stub.Create(context.Background(), &store.APIKeyData{ + ID: keyID, + Name: "own-tenant-key", + TenantID: tid, + }) + + m := &APIKeysMethods{apiKeys: stub} + client := gateway.NewTestClient(permissions.RoleAdmin, tid, "user-1") + + m.handleRevoke(wsCallCtx(client), client, buildRevokeRequest(t, keyID)) + + if !stub.wasRevoked(keyID) { + t.Fatalf("own-tenant key must be revocable by same-tenant admin") + } +} + +// TestWSRevoke_SystemOwner_CrossTenantKey_Allowed documents the intentional +// behaviour: a system owner narrowed to the master scope can still revoke +// keys belonging to other tenants (operator-level housekeeping). If this +// is ever tightened, update both the test and the handler comment. +func TestWSRevoke_SystemOwner_CrossTenantKey_Allowed(t *testing.T) { + otherTID := uuid.MustParse("55555555-5555-5555-5555-555555555555") + keyID := uuid.New() + + stub := newStubAPIKeyStore() + _ = stub.Create(context.Background(), &store.APIKeyData{ + ID: keyID, + Name: "tenant-specific-key", + TenantID: otherTID, + }) + + m := &APIKeysMethods{apiKeys: stub} + client := gateway.NewTestClient(permissions.RoleOwner, store.MasterTenantID, "owner-1") + + m.handleRevoke(wsCallCtx(client), client, buildRevokeRequest(t, keyID)) + + if !stub.wasRevoked(keyID) { + t.Fatalf("system owner must be allowed to revoke cross-tenant keys") + } +} diff --git a/internal/gateway/methods/config.go b/internal/gateway/methods/config.go index 6fd07206..5712d64e 100644 --- a/internal/gateway/methods/config.go +++ b/internal/gateway/methods/config.go @@ -5,7 +5,6 @@ import ( "encoding/json" "log/slog" - "github.com/google/uuid" "github.com/titanous/json5" "github.com/nextlevelbuilder/goclaw/internal/bus" @@ -65,9 +64,12 @@ func (m *ConfigMethods) requireOwner(next gateway.MethodHandler) gateway.MethodH // on-disk config.json. A non-master tenant admin calling config.patch would // corrupt master state + leak master config to other tenants. This guard keeps // config.* strictly master-scoped until a tenant-aware refactor lands. +// +// Shares the predicate with store.IsMasterScope so HTTP and WS layers can't +// drift — same rule, one source of truth. func (m *ConfigMethods) requireMasterScope(next gateway.MethodHandler) gateway.MethodHandler { return func(ctx context.Context, client *gateway.Client, req *protocol.RequestFrame) { - if !isMasterScopeContext(ctx) { + if !store.IsMasterScope(ctx) { locale := store.LocaleFromContext(ctx) client.SendResponse(protocol.NewErrorResponse( req.ID, @@ -80,18 +82,6 @@ func (m *ConfigMethods) requireMasterScope(next gateway.MethodHandler) gateway.M } } -// isMasterScopeContext returns true when ctx should be treated as master-scope: -// (a) system owner role (bypass-all), or -// (b) tenant id is unset (uuid.Nil — legacy / system callers), or -// (c) tenant id equals store.MasterTenantID. -func isMasterScopeContext(ctx context.Context) bool { - if store.IsOwnerRole(ctx) { - return true - } - tid := store.TenantIDFromContext(ctx) - return tid == uuid.Nil || tid == store.MasterTenantID -} - func (m *ConfigMethods) handleGet(_ context.Context, client *gateway.Client, req *protocol.RequestFrame) { client.SendResponse(protocol.NewOKResponse(req.ID, map[string]any{ "config": m.cfg.MaskedCopy(), diff --git a/internal/gateway/methods/config_tenant_guard_test.go b/internal/gateway/methods/config_tenant_guard_test.go index 5aa003cd..5825264e 100644 --- a/internal/gateway/methods/config_tenant_guard_test.go +++ b/internal/gateway/methods/config_tenant_guard_test.go @@ -11,13 +11,13 @@ import ( "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) -// ---- Tests: isMasterScopeContext helper ---- +// ---- Tests: store.IsMasterScope helper ---- func TestIsMasterScopeContext_OwnerRole_Allowed(t *testing.T) { ctx := store.WithRole(context.Background(), store.RoleOwner) // Tenant set to a non-master tenant — owner role must still pass ctx = store.WithTenantID(ctx, uuid.MustParse("11111111-1111-1111-1111-111111111111")) - if !isMasterScopeContext(ctx) { + if !store.IsMasterScope(ctx) { t.Fatalf("owner role with non-master tenant should be allowed") } } @@ -25,14 +25,14 @@ func TestIsMasterScopeContext_OwnerRole_Allowed(t *testing.T) { func TestIsMasterScopeContext_NilTenant_Allowed(t *testing.T) { // Legacy / system callers without tenant scope → treated as master ctx := context.Background() - if !isMasterScopeContext(ctx) { + if !store.IsMasterScope(ctx) { t.Fatalf("nil tenant ctx should be allowed (master-scope fallback)") } } func TestIsMasterScopeContext_MasterTenant_Allowed(t *testing.T) { ctx := store.WithTenantID(context.Background(), store.MasterTenantID) - if !isMasterScopeContext(ctx) { + if !store.IsMasterScope(ctx) { t.Fatalf("master tenant ctx should be allowed") } } @@ -40,7 +40,7 @@ func TestIsMasterScopeContext_MasterTenant_Allowed(t *testing.T) { func TestIsMasterScopeContext_NonMasterTenantNoOwner_Denied(t *testing.T) { tid := uuid.MustParse("22222222-2222-2222-2222-222222222222") ctx := store.WithTenantID(context.Background(), tid) - if isMasterScopeContext(ctx) { + if store.IsMasterScope(ctx) { t.Fatalf("non-master tenant ctx without owner role must be denied") } } @@ -50,7 +50,7 @@ func TestIsMasterScopeContext_NonMasterTenantWithOwnerRole_Allowed(t *testing.T) tid := uuid.MustParse("33333333-3333-3333-3333-333333333333") ctx := store.WithTenantID(context.Background(), tid) ctx = store.WithRole(ctx, store.RoleOwner) - if !isMasterScopeContext(ctx) { + if !store.IsMasterScope(ctx) { t.Fatalf("owner role must bypass tenant scope check") } } diff --git a/internal/gateway/router.go b/internal/gateway/router.go index 5972bf7b..0ac57b02 100644 --- a/internal/gateway/router.go +++ b/internal/gateway/router.go @@ -78,8 +78,11 @@ func (r *MethodRouter) Handle(ctx context.Context, client *Client, req *protocol } } - // Inject locale + tenant into context. + // Inject locale + tenant + role into context. // All connect paths guarantee client.tenantID is set (owner defaults to MasterTenantID). + // Role injection is required so store.IsOwnerRole / store.IsMasterScope work + // from WS handlers — without it, ctx-based permission helpers silently + // evaluate as non-owner. HTTP layer does the same via enrichContext. ctx = store.WithLocale(ctx, i18n.Normalize(client.locale)) if client.TenantID() != uuid.Nil { ctx = store.WithTenantID(ctx, client.TenantID()) @@ -87,6 +90,9 @@ func (r *MethodRouter) Handle(ctx context.Context, client *Client, req *protocol if slug := client.TenantSlug(); slug != "" { ctx = store.WithTenantSlug(ctx, slug) } + if role := client.Role(); role != "" { + ctx = store.WithRole(ctx, string(role)) + } slog.Debug("handling method", "method", req.Method, "client", client.id, "req_id", req.ID) handler(ctx, client, req) diff --git a/internal/http/api_key_cache_test.go b/internal/http/api_key_cache_test.go index 6cc7dca4..ecd920ca 100644 --- a/internal/http/api_key_cache_test.go +++ b/internal/http/api_key_cache_test.go @@ -16,12 +16,25 @@ import ( type mockAPIKeyStore struct { mu sync.Mutex keys map[string]*store.APIKeyData // hash → key - calls int // GetByHash call count - touchedID uuid.UUID // last TouchLastUsed ID + 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)} + 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) { @@ -46,7 +59,6 @@ func (m *mockAPIKeyStore) List(_ context.Context, _ string) ([]store.APIKeyData, return nil, nil } func (m *mockAPIKeyStore) Revoke(_ context.Context, _ uuid.UUID, _ string) error { return nil } -func (m *mockAPIKeyStore) Delete(_ context.Context, _ uuid.UUID, _ string) error { return nil } func (m *mockAPIKeyStore) getCalls() int { m.mu.Lock() diff --git a/internal/http/api_keys.go b/internal/http/api_keys.go index c9ad983f..3be1245f 100644 --- a/internal/http/api_keys.go +++ b/internal/http/api_keys.go @@ -148,6 +148,14 @@ func (h *APIKeysHandler) handleCreate(w http.ResponseWriter, r *http.Request) { }) } +// handleRevoke revokes an API key after verifying the caller owns it. +// +// Phase 0b hotfix: the store layer's Revoke SQL matches rows where +// `tenant_id = $N OR tenant_id IS NULL`, which let tenant admins revoke +// system-level (NULL-tenant) API keys belonging to CI/CD or integrations. +// The fix pre-fetches the key and rejects any non-owner caller whose tenant +// does not exactly match the key's tenant. System owners bypass. +// See plans/reports/debugger-260412-0922-tenant-scope-audit.md HIGH finding. func (h *APIKeysHandler) handleRevoke(w http.ResponseWriter, r *http.Request) { locale := extractLocale(r) idStr := r.PathValue("id") @@ -157,8 +165,35 @@ func (h *APIKeysHandler) handleRevoke(w http.ResponseWriter, r *http.Request) { return } + ctx := r.Context() + + // Fetch first so ownership can be verified before any mutation. + key, err := h.apiKeys.Get(ctx, id) + if err != nil || key == nil { + writeError(w, http.StatusNotFound, protocol.ErrNotFound, i18n.T(locale, i18n.MsgNotFound, "API key", idStr)) + return + } + + // Ownership rule: + // - System owner (bypass-all) → may revoke any key + // - Tenant admin in caller's tenant → may revoke only keys owned by that tenant (strict match) + // - NULL-tenant (system) keys → revocable only by system owners + if !store.IsOwnerRole(ctx) { + callerTID := store.TenantIDFromContext(ctx) + if key.TenantID == uuid.Nil || key.TenantID != callerTID { + slog.Warn("security.api_key_revoke_forbidden", + "key_id", idStr, + "caller_tenant", callerTID, + "key_tenant", key.TenantID, + "user_id", store.UserIDFromContext(ctx), + ) + writeError(w, http.StatusForbidden, protocol.ErrUnauthorized, i18n.T(locale, i18n.MsgPermissionDenied, "API key")) + return + } + } + // HTTP revoke is admin-only (adminAuth middleware), so no owner filter needed. - if err := h.apiKeys.Revoke(r.Context(), id, ""); err != nil { + if err := h.apiKeys.Revoke(ctx, id, ""); err != nil { slog.Error("api_keys.revoke failed", "error", err, "id", idStr) writeError(w, http.StatusNotFound, protocol.ErrNotFound, i18n.T(locale, i18n.MsgNotFound, "API key", idStr)) return diff --git a/internal/http/builtin_tools.go b/internal/http/builtin_tools.go index b5f28f88..106ddc3e 100644 --- a/internal/http/builtin_tools.go +++ b/internal/http/builtin_tools.go @@ -119,6 +119,13 @@ func (h *BuiltinToolsHandler) handleGet(w http.ResponseWriter, r *http.Request) } func (h *BuiltinToolsHandler) handleUpdate(w http.ResponseWriter, r *http.Request) { + // Phase 0b hotfix: builtin_tools is a global table with no tenant_id column, + // so this write must be restricted to master-scope callers. Non-master tenant + // admins must go through PUT /v1/tools/builtin/{name}/tenant-config instead. + // See plans/reports/debugger-260412-0922-tenant-scope-audit.md CRITICAL-1. + if !requireMasterScope(w, r) { + return + } locale := extractLocale(r) name := r.PathValue("name") if name == "" { diff --git a/internal/http/packages.go b/internal/http/packages.go index 126feb4c..bf2a4451 100644 --- a/internal/http/packages.go +++ b/internal/http/packages.go @@ -81,7 +81,15 @@ func parseAndValidatePackage(w http.ResponseWriter, r *http.Request) string { // handleInstall installs a single package. // Body: {"package": "github-cli"} or {"package": "pip:pandas"} or {"package": "npm:typescript"} +// +// Phase 0b hotfix: server-wide package installation (pip/npm/apk) must be +// restricted to master-scope callers. Non-master tenant admins previously +// reached this handler because the adminAuth middleware only checks role, not +// tenant scope — a supply-chain vector (CRITICAL-2 in the audit report). func (h *PackagesHandler) handleInstall(w http.ResponseWriter, r *http.Request) { + if !requireMasterScope(w, r) { + return + } pkg := parseAndValidatePackage(w, r) if pkg == "" { return @@ -96,7 +104,13 @@ func (h *PackagesHandler) handleInstall(w http.ResponseWriter, r *http.Request) // handleUninstall removes a single package. // Body: {"package": "github-cli"} or {"package": "pip:pandas"} or {"package": "npm:typescript"} +// +// Phase 0b hotfix: same master-scope guard as handleInstall — uninstall can +// break system skills, causing server-wide DoS for every tenant. func (h *PackagesHandler) handleUninstall(w http.ResponseWriter, r *http.Request) { + if !requireMasterScope(w, r) { + return + } pkg := parseAndValidatePackage(w, r) if pkg == "" { return diff --git a/internal/http/tenant_auth_helpers.go b/internal/http/tenant_auth_helpers.go index 404739df..3b40fc81 100644 --- a/internal/http/tenant_auth_helpers.go +++ b/internal/http/tenant_auth_helpers.go @@ -1,6 +1,7 @@ package http import ( + "log/slog" "net/http" "github.com/google/uuid" @@ -53,3 +54,34 @@ func requireTenantAdmin(w http.ResponseWriter, r *http.Request, ts store.TenantS } 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 +} diff --git a/internal/http/tenant_scope_hotfix_test.go b/internal/http/tenant_scope_hotfix_test.go new file mode 100644 index 00000000..b90c201a --- /dev/null +++ b/internal/http/tenant_scope_hotfix_test.go @@ -0,0 +1,345 @@ +package http + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// Phase 0b hotfix coverage: +// - requireMasterScope unit tests (predicate surface) +// - builtin_tools handleUpdate: CRITICAL-1 regression guard +// - packages handleInstall / handleUninstall: CRITICAL-2 regression guard +// - api_keys handleRevoke: HIGH finding regression guard +// +// All tests use focused harnesses that avoid DB / real installers. + +// ---- helpers ---- + +func newMasterScopeReq(method, path string, tid uuid.UUID, role string) *http.Request { + r := httptest.NewRequest(method, path, strings.NewReader("")) + ctx := r.Context() + if tid != uuid.Nil { + ctx = store.WithTenantID(ctx, tid) + } + if role != "" { + ctx = store.WithRole(ctx, role) + } + return r.WithContext(ctx) +} + +// ---- requireMasterScope predicate tests ---- + +func TestRequireMasterScope_SystemOwner_Allows(t *testing.T) { + // Owner role on arbitrary tenant must pass. + r := newMasterScopeReq(http.MethodPut, "/test", uuid.New(), store.RoleOwner) + w := httptest.NewRecorder() + if !requireMasterScope(w, r) { + t.Fatalf("system owner must be allowed, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestRequireMasterScope_MasterTenant_Allows(t *testing.T) { + r := newMasterScopeReq(http.MethodPut, "/test", store.MasterTenantID, "admin") + w := httptest.NewRecorder() + if !requireMasterScope(w, r) { + t.Fatalf("master tenant ctx must be allowed, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestRequireMasterScope_NilTenant_Allows(t *testing.T) { + // Legacy / system callers without any tenant ctx. + r := newMasterScopeReq(http.MethodPut, "/test", uuid.Nil, "") + w := httptest.NewRecorder() + if !requireMasterScope(w, r) { + t.Fatalf("nil tenant ctx must be allowed (master-scope fallback), got %d", w.Code) + } +} + +func TestRequireMasterScope_NonMasterAdmin_Rejects(t *testing.T) { + // The bug-fix path: non-master tenant admin must be rejected. + tid := uuid.MustParse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa") + r := newMasterScopeReq(http.MethodPut, "/test", tid, "admin") + w := httptest.NewRecorder() + if requireMasterScope(w, r) { + t.Fatalf("non-master admin ctx must NOT be allowed") + } + if w.Code != http.StatusForbidden { + t.Fatalf("expected 403, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "master") { + t.Errorf("expected error message to mention master scope, got: %s", w.Body.String()) + } +} + +// ---- CRITICAL-1: builtin_tools handleUpdate regression ---- + +// TestBuiltinToolsUpdate_RejectsNonMasterAdmin verifies that Phase 0b guard +// blocks the exact attack vector described in the audit: a tenant admin +// (RoleAdmin in a non-master tenant) attempting PUT /v1/tools/builtin/{name} +// receives 403 without any store mutation. +func TestBuiltinToolsUpdate_RejectsNonMasterAdmin(t *testing.T) { + h := &BuiltinToolsHandler{} // store nil — must never be reached + mux := http.NewServeMux() + mux.HandleFunc("PUT /v1/tools/builtin/{name}", h.handleUpdate) + + tid := uuid.MustParse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb") + req := httptest.NewRequest(http.MethodPut, "/v1/tools/builtin/web_search", + strings.NewReader(`{"enabled":true}`)) + req.Header.Set("Content-Type", "application/json") + ctx := store.WithTenantID(req.Context(), tid) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + // If the guard fails open, h.store.Update is called on a nil store and + // the test panics — explicit recover catches that regression distinctly. + defer func() { + if r := recover(); r != nil { + t.Fatalf("guard did not short-circuit — handler panicked on nil store: %v", r) + } + }() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for non-master admin, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// ---- CRITICAL-2: packages handleInstall / handleUninstall regression ---- + +func TestPackagesInstall_RejectsNonMasterAdmin(t *testing.T) { + h := NewPackagesHandler() + mux := http.NewServeMux() + mux.HandleFunc("POST /v1/packages/install", h.handleInstall) + + tid := uuid.MustParse("cccccccc-cccc-cccc-cccc-cccccccccccc") + req := httptest.NewRequest(http.MethodPost, "/v1/packages/install", + strings.NewReader(`{"package":"pip:malicious-pkg"}`)) + req.Header.Set("Content-Type", "application/json") + ctx := store.WithTenantID(req.Context(), tid) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for non-master admin, got %d: %s", rec.Code, rec.Body.String()) + } +} + +func TestPackagesUninstall_RejectsNonMasterAdmin(t *testing.T) { + h := NewPackagesHandler() + mux := http.NewServeMux() + mux.HandleFunc("POST /v1/packages/uninstall", h.handleUninstall) + + tid := uuid.MustParse("dddddddd-dddd-dddd-dddd-dddddddddddd") + req := httptest.NewRequest(http.MethodPost, "/v1/packages/uninstall", + strings.NewReader(`{"package":"pip:pandas"}`)) + req.Header.Set("Content-Type", "application/json") + ctx := store.WithTenantID(req.Context(), tid) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for non-master admin, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// ---- HIGH: api_keys handleRevoke ownership regression ---- + +// TestAPIKeyRevoke_RejectsCrossTenant exercises the post-fix ownership check +// in the HTTP handler. A tenant admin (non-owner) attempting to revoke a +// key owned by a different tenant must receive 403, and the store-layer +// Revoke must never be called — mockAPIKeyStore flags when it is. +func TestAPIKeyRevoke_RejectsCrossTenant(t *testing.T) { + ms := newMockAPIKeyStore() + callerTID := uuid.MustParse("eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee") + keyOwnerTID := uuid.MustParse("ffffffff-ffff-ffff-ffff-ffffffffffff") + keyID := uuid.New() + ms.byID[keyID] = &store.APIKeyData{ + ID: keyID, + Name: "other-tenant-key", + TenantID: keyOwnerTID, + } + + h := &APIKeysHandler{apiKeys: ms} + mux := http.NewServeMux() + mux.HandleFunc("POST /v1/api-keys/{id}/revoke", h.handleRevoke) + + req := httptest.NewRequest(http.MethodPost, "/v1/api-keys/"+keyID.String()+"/revoke", nil) + ctx := store.WithTenantID(req.Context(), callerTID) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for cross-tenant revoke, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// TestAPIKeyRevoke_RejectsSystemKeyForTenantAdmin verifies the NULL-tenant +// (system-level) key cannot be revoked by a tenant admin — the core HIGH +// vulnerability from the audit (store SQL `OR tenant_id IS NULL` arm). +func TestAPIKeyRevoke_RejectsSystemKeyForTenantAdmin(t *testing.T) { + ms := newMockAPIKeyStore() + callerTID := uuid.MustParse("11111111-1111-1111-1111-111111111111") + keyID := uuid.New() + // Key with TenantID == uuid.Nil represents a NULL-tenant system key. + ms.byID[keyID] = &store.APIKeyData{ + ID: keyID, + Name: "system-ci-key", + TenantID: uuid.Nil, + } + + h := &APIKeysHandler{apiKeys: ms} + mux := http.NewServeMux() + mux.HandleFunc("POST /v1/api-keys/{id}/revoke", h.handleRevoke) + + req := httptest.NewRequest(http.MethodPost, "/v1/api-keys/"+keyID.String()+"/revoke", nil) + ctx := store.WithTenantID(req.Context(), callerTID) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for tenant admin revoking system key, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// TestAPIKeyRevoke_AllowsOwnTenantKey happy path — tenant admin revoking a +// key owned by their own tenant must succeed (no regression). +func TestAPIKeyRevoke_AllowsOwnTenantKey(t *testing.T) { + ms := newMockAPIKeyStore() + tid := uuid.MustParse("22222222-2222-2222-2222-222222222222") + keyID := uuid.New() + ms.byID[keyID] = &store.APIKeyData{ + ID: keyID, + Name: "own-tenant-key", + TenantID: tid, + } + + h := &APIKeysHandler{apiKeys: ms} + mux := http.NewServeMux() + mux.HandleFunc("POST /v1/api-keys/{id}/revoke", h.handleRevoke) + + req := httptest.NewRequest(http.MethodPost, "/v1/api-keys/"+keyID.String()+"/revoke", nil) + ctx := store.WithTenantID(req.Context(), tid) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 for same-tenant revoke, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// TestAPIKeyRevoke_AllowsSystemOwnerOnSystemKey happy path — system owner +// can revoke NULL-tenant system keys (CI/CD rotation). +func TestAPIKeyRevoke_AllowsSystemOwnerOnSystemKey(t *testing.T) { + ms := newMockAPIKeyStore() + keyID := uuid.New() + ms.byID[keyID] = &store.APIKeyData{ + ID: keyID, + Name: "system-ci-key", + TenantID: uuid.Nil, + } + + h := &APIKeysHandler{apiKeys: ms} + mux := http.NewServeMux() + mux.HandleFunc("POST /v1/api-keys/{id}/revoke", h.handleRevoke) + + req := httptest.NewRequest(http.MethodPost, "/v1/api-keys/"+keyID.String()+"/revoke", nil) + ctx := store.WithRole(req.Context(), store.RoleOwner) + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 for system owner revoking system key, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// --- master-scope happy paths on guarded handlers --- + +// recordingBuiltinToolStore is a minimal BuiltinToolStore stub that records +// calls to Update so the happy-path test can assert "the guard allowed the +// request through to the store" without relying on panic-recover semantics. +// Only Update is exercised; the rest return zero values to satisfy the +// interface contract. +type recordingBuiltinToolStore struct { + updateName string + updateBody map[string]any +} + +func (s *recordingBuiltinToolStore) Update(_ context.Context, name string, updates map[string]any) error { + s.updateName = name + s.updateBody = updates + return nil +} +func (s *recordingBuiltinToolStore) List(_ context.Context) ([]store.BuiltinToolDef, error) { + return nil, nil +} +func (s *recordingBuiltinToolStore) Get(_ context.Context, _ string) (*store.BuiltinToolDef, error) { + return nil, nil +} +func (s *recordingBuiltinToolStore) Seed(_ context.Context, _ []store.BuiltinToolDef) error { + return nil +} +func (s *recordingBuiltinToolStore) ListEnabled(_ context.Context) ([]store.BuiltinToolDef, error) { + return nil, nil +} +func (s *recordingBuiltinToolStore) GetSettings(_ context.Context, _ string) (json.RawMessage, error) { + return nil, nil +} + +// TestBuiltinToolsUpdate_AllowsMasterAdmin ensures master-scope callers are +// not blocked by Phase 0b. Uses a recording stub store so we can assert the +// guard passed and the handler reached Update — no fragile panic-recover. +func TestBuiltinToolsUpdate_AllowsMasterAdmin(t *testing.T) { + rec := &recordingBuiltinToolStore{} + h := &BuiltinToolsHandler{store: rec} + mux := http.NewServeMux() + mux.HandleFunc("PUT /v1/tools/builtin/{name}", h.handleUpdate) + + req := httptest.NewRequest(http.MethodPut, "/v1/tools/builtin/web_search", + strings.NewReader(`{"enabled":true}`)) + req.Header.Set("Content-Type", "application/json") + ctx := store.WithTenantID(req.Context(), store.MasterTenantID) + ctx = store.WithRole(ctx, "admin") + req = req.WithContext(ctx) + + rr := httptest.NewRecorder() + mux.ServeHTTP(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200 for master admin, got %d: %s", rr.Code, rr.Body.String()) + } + if rec.updateName != "web_search" { + t.Errorf("expected Update called with name=web_search, got %q", rec.updateName) + } + if enabled, ok := rec.updateBody["enabled"].(bool); !ok || !enabled { + t.Errorf("expected Update body {enabled:true}, got %+v", rec.updateBody) + } +} + +// sanity: make sure json is importable (unused import guard during refactors) +var _ = json.Marshal diff --git a/internal/i18n/catalog_en.go b/internal/i18n/catalog_en.go index fd6e33c4..df79637f 100644 --- a/internal/i18n/catalog_en.go +++ b/internal/i18n/catalog_en.go @@ -108,6 +108,7 @@ func init() { MsgRawConfigRequired: "raw config is required", MsgRawPatchRequired: "raw patch is required", MsgConfigMasterScopeOnly: "config.* methods are master-scope only; use tenant tool config endpoints for per-tenant overrides", + MsgMasterScopeRequired: "this action requires master tenant scope", // Storage / File MsgCannotDeleteSkillsDir: "cannot delete skills directories", diff --git a/internal/i18n/catalog_vi.go b/internal/i18n/catalog_vi.go index 1f2227b0..39cebfab 100644 --- a/internal/i18n/catalog_vi.go +++ b/internal/i18n/catalog_vi.go @@ -108,6 +108,7 @@ func init() { MsgRawConfigRequired: "cấu hình raw là bắt buộc", MsgRawPatchRequired: "patch raw là bắt buộc", MsgConfigMasterScopeOnly: "config.* chỉ áp dụng cho master scope; dùng endpoint tenant tool config cho override theo tenant", + MsgMasterScopeRequired: "thao tác này yêu cầu phạm vi tenant chính", // Storage / File MsgCannotDeleteSkillsDir: "không thể xóa thư mục skill", diff --git a/internal/i18n/catalog_zh.go b/internal/i18n/catalog_zh.go index 61ddfa7d..627809c9 100644 --- a/internal/i18n/catalog_zh.go +++ b/internal/i18n/catalog_zh.go @@ -108,6 +108,7 @@ func init() { MsgRawConfigRequired: "raw 配置是必填项", MsgRawPatchRequired: "raw 补丁是必填项", MsgConfigMasterScopeOnly: "config.* 方法仅适用于主作用域;使用租户工具配置端点进行租户级覆盖", + MsgMasterScopeRequired: "此操作需要主租户范围", // Storage / File MsgCannotDeleteSkillsDir: "无法删除Skill目录", diff --git a/internal/i18n/keys.go b/internal/i18n/keys.go index a84bd22d..a7890574 100644 --- a/internal/i18n/keys.go +++ b/internal/i18n/keys.go @@ -109,6 +109,7 @@ const ( MsgRawConfigRequired = "error.raw_config_required" // "raw config is required" MsgRawPatchRequired = "error.raw_patch_required" // "raw patch is required" MsgConfigMasterScopeOnly = "error.config_master_scope_only" // "config.* methods are master-scope only" + MsgMasterScopeRequired = "error.master_scope_required" // "this action requires master tenant scope" // --- Storage / File --- MsgCannotDeleteSkillsDir = "error.cannot_delete_skills_dir" // "cannot delete skills directories" diff --git a/internal/store/api_key_store.go b/internal/store/api_key_store.go index 039b3b0a..441f4022 100644 --- a/internal/store/api_key_store.go +++ b/internal/store/api_key_store.go @@ -29,6 +29,13 @@ type APIKeyStore interface { // Create inserts a new API key. Create(ctx context.Context, key *APIKeyData) error + // Get looks up an API key by its ID. Unlike GetByHash, this does NOT + // filter on revoked/expired state — used by admin handlers that need to + // verify ownership before revoke/delete. Returns sql.ErrNoRows when the + // key does not exist. No tenant scoping is applied at store level — + // callers must enforce their own ownership rules. + Get(ctx context.Context, id uuid.UUID) (*APIKeyData, error) + // GetByHash looks up an active (non-revoked, non-expired) key by its SHA-256 hash. GetByHash(ctx context.Context, keyHash string) (*APIKeyData, error) @@ -38,9 +45,6 @@ type APIKeyStore interface { // Revoke marks a key as revoked. If ownerID is non-empty, also enforces owner_id = ownerID. Revoke(ctx context.Context, id uuid.UUID, ownerID string) error - // Delete permanently removes a key. If ownerID is non-empty, also enforces owner_id = ownerID. - Delete(ctx context.Context, id uuid.UUID, ownerID string) error - // TouchLastUsed updates the last_used_at timestamp. TouchLastUsed(ctx context.Context, id uuid.UUID) error } diff --git a/internal/store/context.go b/internal/store/context.go index 633ce8c9..94122f58 100644 --- a/internal/store/context.go +++ b/internal/store/context.go @@ -287,6 +287,24 @@ func IsOwnerRole(ctx context.Context) bool { return RoleFromContext(ctx) == string(RoleOwner) } +// IsMasterScope reports whether ctx should be treated as master-scope: +// +// (a) system owner role (IsOwnerRole bypass-all), or +// (b) tenant id is unset (uuid.Nil — legacy / system callers), or +// (c) tenant id equals MasterTenantID. +// +// Used by both WS config.* methods and HTTP admin routes that write to +// global (non-tenant-scoped) tables or execute server-wide side effects +// (shell, filesystem). Centralises the Phase 1 / Phase 0b hotfix rule +// so every layer shares one predicate — no drift. +func IsMasterScope(ctx context.Context) bool { + if IsOwnerRole(ctx) { + return true + } + tid := TenantIDFromContext(ctx) + return tid == uuid.Nil || tid == MasterTenantID +} + // RoleOwner is the owner role constant for context checks. // Must match permissions.RoleOwner. const RoleOwner = "owner" diff --git a/internal/store/pg/api_keys.go b/internal/store/pg/api_keys.go index d6248d5f..b626bfef 100644 --- a/internal/store/pg/api_keys.go +++ b/internal/store/pg/api_keys.go @@ -41,6 +41,40 @@ func (s *PGAPIKeyStore) Create(ctx context.Context, key *store.APIKeyData) error return err } +// Get fetches a key by ID without revoked/expired filtering. No tenant scoping +// at store layer — callers must enforce their own ownership rules. +func (s *PGAPIKeyStore) Get(ctx context.Context, id uuid.UUID) (*store.APIKeyData, error) { + row := s.db.QueryRowContext(ctx, + `SELECT id, name, prefix, key_hash, scopes, owner_id, tenant_id, expires_at, last_used_at, revoked, created_by, created_at, updated_at + FROM api_keys + WHERE id = $1`, + id, + ) + + var k store.APIKeyData + var createdBy *string + var ownerID *string + var tenantID *uuid.UUID + err := row.Scan( + &k.ID, &k.Name, &k.Prefix, &k.KeyHash, pq.Array(&k.Scopes), + &ownerID, &tenantID, &k.ExpiresAt, &k.LastUsedAt, &k.Revoked, &createdBy, + &k.CreatedAt, &k.UpdatedAt, + ) + if err != nil { + return nil, err + } + if createdBy != nil { + k.CreatedBy = *createdBy + } + if ownerID != nil { + k.OwnerID = *ownerID + } + if tenantID != nil { + k.TenantID = *tenantID + } + return &k, nil +} + func (s *PGAPIKeyStore) GetByHash(ctx context.Context, keyHash string) (*store.APIKeyData, error) { row := s.db.QueryRowContext(ctx, `SELECT id, name, prefix, key_hash, scopes, owner_id, tenant_id, expires_at, last_used_at, revoked, created_by, created_at, updated_at @@ -167,35 +201,6 @@ func (s *PGAPIKeyStore) Revoke(ctx context.Context, id uuid.UUID, ownerID string return nil } -func (s *PGAPIKeyStore) Delete(ctx context.Context, id uuid.UUID, ownerID string) error { - q := "DELETE FROM api_keys WHERE id = $1" - args := []any{id} - argIdx := 2 - - if ownerID != "" { - q += fmt.Sprintf(" AND owner_id = $%d", argIdx) - args = append(args, ownerID) - argIdx++ - } - if !store.IsCrossTenant(ctx) { - tid := store.TenantIDFromContext(ctx) - if tid != uuid.Nil { - q += fmt.Sprintf(" AND (tenant_id = $%d OR tenant_id IS NULL)", argIdx) - args = append(args, tid) - } - } - - res, err := s.db.ExecContext(ctx, q, args...) - if err != nil { - return err - } - n, _ := res.RowsAffected() - if n == 0 { - return sql.ErrNoRows - } - return nil -} - func (s *PGAPIKeyStore) TouchLastUsed(ctx context.Context, id uuid.UUID) error { _, err := s.db.ExecContext(ctx, `UPDATE api_keys SET last_used_at = $2 WHERE id = $1`, diff --git a/internal/store/sqlitestore/api_keys.go b/internal/store/sqlitestore/api_keys.go index 11aeeafc..4b219538 100644 --- a/internal/store/sqlitestore/api_keys.go +++ b/internal/store/sqlitestore/api_keys.go @@ -41,6 +41,52 @@ func (s *SQLiteAPIKeyStore) Create(ctx context.Context, key *store.APIKeyData) e return err } +// Get fetches a key by ID without revoked/expired filtering. No tenant scoping +// at store layer — callers must enforce their own ownership rules. +func (s *SQLiteAPIKeyStore) Get(ctx context.Context, id uuid.UUID) (*store.APIKeyData, error) { + row := s.db.QueryRowContext(ctx, + `SELECT id, name, prefix, key_hash, scopes, owner_id, tenant_id, expires_at, last_used_at, revoked, created_by, created_at, updated_at + FROM api_keys + WHERE id = ?`, + id, + ) + + var k store.APIKeyData + var createdBy *string + var ownerID *string + var tenantID *uuid.UUID + var scopesRaw []byte + var expiresAt, lastUsedAt nullSqliteTime + createdAt, updatedAt := scanTimePair() + err := row.Scan( + &k.ID, &k.Name, &k.Prefix, &k.KeyHash, &scopesRaw, + &ownerID, &tenantID, &expiresAt, &lastUsedAt, &k.Revoked, &createdBy, + createdAt, updatedAt, + ) + if err != nil { + return nil, err + } + k.CreatedAt = createdAt.Time + k.UpdatedAt = updatedAt.Time + if expiresAt.Valid { + k.ExpiresAt = &expiresAt.Time + } + if lastUsedAt.Valid { + k.LastUsedAt = &lastUsedAt.Time + } + scanJSONStringArray(scopesRaw, &k.Scopes) + if createdBy != nil { + k.CreatedBy = *createdBy + } + if ownerID != nil { + k.OwnerID = *ownerID + } + if tenantID != nil { + k.TenantID = *tenantID + } + return &k, nil +} + func (s *SQLiteAPIKeyStore) GetByHash(ctx context.Context, keyHash string) (*store.APIKeyData, error) { row := s.db.QueryRowContext(ctx, `SELECT id, name, prefix, key_hash, scopes, owner_id, tenant_id, expires_at, last_used_at, revoked, created_by, created_at, updated_at @@ -185,33 +231,6 @@ func (s *SQLiteAPIKeyStore) Revoke(ctx context.Context, id uuid.UUID, ownerID st return nil } -func (s *SQLiteAPIKeyStore) Delete(ctx context.Context, id uuid.UUID, ownerID string) error { - q := "DELETE FROM api_keys WHERE id = ?" - args := []any{id} - - if ownerID != "" { - q += " AND owner_id = ?" - args = append(args, ownerID) - } - if !store.IsCrossTenant(ctx) { - tid := store.TenantIDFromContext(ctx) - if tid != uuid.Nil { - q += " AND (tenant_id = ? OR tenant_id IS NULL)" - args = append(args, tid) - } - } - - res, err := s.db.ExecContext(ctx, q, args...) - if err != nil { - return err - } - n, _ := res.RowsAffected() - if n == 0 { - return sql.ErrNoRows - } - return nil -} - func (s *SQLiteAPIKeyStore) TouchLastUsed(ctx context.Context, id uuid.UUID) error { _, err := s.db.ExecContext(ctx, `UPDATE api_keys SET last_used_at = ? WHERE id = ?`,