diff --git a/internal/http/secure_cli_agent_grants.go b/internal/http/secure_cli_agent_grants.go index 9fe14713..11f87289 100644 --- a/internal/http/secure_cli_agent_grants.go +++ b/internal/http/secure_cli_agent_grants.go @@ -137,6 +137,33 @@ func validateAndSerializeEnvVars(w http.ResponseWriter, locale string, envVars m return b, true } +func parseGrantPathIDs(w http.ResponseWriter, r *http.Request, locale string) (uuid.UUID, uuid.UUID, bool) { + binaryID, err := uuid.Parse(r.PathValue("id")) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "credential")}) + return uuid.Nil, uuid.Nil, false + } + grantID, err := uuid.Parse(r.PathValue("grantId")) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "grant")}) + return uuid.Nil, uuid.Nil, false + } + return binaryID, grantID, true +} + +func (h *SecureCLIGrantHandler) getGrantForBinary(w http.ResponseWriter, r *http.Request, locale string) (*store.SecureCLIAgentGrant, uuid.UUID, bool) { + binaryID, grantID, ok := parseGrantPathIDs(w, r, locale) + if !ok { + return nil, uuid.Nil, false + } + g, err := h.grants.Get(r.Context(), grantID) + if err != nil || g.BinaryID != binaryID { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "grant", grantID.String())}) + return nil, uuid.Nil, false + } + return g, binaryID, true +} + func (h *SecureCLIGrantHandler) handleList(w http.ResponseWriter, r *http.Request) { if !requireTenantAdmin(w, r, h.tenantStore) { return @@ -180,6 +207,22 @@ func (h *SecureCLIGrantHandler) handleCreate(w http.ResponseWriter, r *http.Requ writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgRequired, "agent_id")}) return } + if exists, err := h.grants.BinaryExists(r.Context(), binaryID); err != nil { + slog.Error("secure_cli_grants.create.binary_scope", "binary_id", binaryID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "validate credential")}) + return + } else if !exists { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "credential", binaryID.String())}) + return + } + if exists, err := h.grants.AgentExists(r.Context(), req.AgentID); err != nil { + slog.Error("secure_cli_grants.create.agent_scope", "agent_id", req.AgentID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "validate agent")}) + return + } else if !exists { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "agent", req.AgentID.String())}) + return + } enabled := true if req.Enabled != nil { @@ -243,14 +286,8 @@ func (h *SecureCLIGrantHandler) handleGet(w http.ResponseWriter, r *http.Request return } locale := store.LocaleFromContext(r.Context()) - grantID, err := uuid.Parse(r.PathValue("grantId")) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "grant")}) - return - } - g, err := h.grants.Get(r.Context(), grantID) - if err != nil { - writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "grant", grantID.String())}) + g, _, ok := h.getGrantForBinary(w, r, locale) + if !ok { return } populateGrantEnvFields(g) @@ -262,9 +299,8 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ return } locale := store.LocaleFromContext(r.Context()) - grantID, err := uuid.Parse(r.PathValue("grantId")) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "grant")}) + g, binaryID, ok := h.getGrantForBinary(w, r, locale) + if !ok { return } @@ -298,16 +334,13 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ updates[k] = decoded } } - if err := h.grants.Update(r.Context(), grantID, updates); err != nil { - slog.Error("secure_cli_grants.update", "grant_id", grantID, "error", err) - writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "update grant")}) - return - } - // 3-state env_vars semantics: absent=skip, null=clear, {...}=replace. // Finding #15: {} (empty map) is treated as clear — same as null. // TS type: absent | null | Record — see ui/web/src/types/cli-credential.ts. + var envJSON []byte + envPresent := false if envRaw, present := raw["env_vars"]; present { + envPresent = true var envPtr *map[string]string if string(envRaw) != "null" { var m map[string]string @@ -320,7 +353,6 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ // envPtr == nil → clear; envPtr != nil → replace. // Note: envPtr pointing to an empty map ({}) is treated as clear (same as null) — // envJSON stays nil and UpdateGrantEnv(nil) removes the override. - var envJSON []byte if envPtr != nil && len(*envPtr) > 0 { j, ok := validateAndSerializeEnvVars(w, locale, *envPtr) if !ok { @@ -328,14 +360,23 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ } envJSON = j } - if err := h.grants.UpdateGrantEnv(r.Context(), grantID, envJSON); err != nil { - slog.Error("secure_cli_grants.update.set_env", "grant_id", grantID, "error", err) + } + + if err := h.grants.Update(r.Context(), g.ID, updates); err != nil { + slog.Error("secure_cli_grants.update", "grant_id", g.ID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "update grant")}) + return + } + + if envPresent { + if err := h.grants.UpdateGrantEnv(r.Context(), g.ID, envJSON); err != nil { + slog.Error("secure_cli_grants.update.set_env", "grant_id", g.ID, "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "update grant env")}) return } } - h.emitCacheInvalidate(r.PathValue("id")) + h.emitCacheInvalidate(binaryID.String()) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } @@ -344,18 +385,17 @@ func (h *SecureCLIGrantHandler) handleDelete(w http.ResponseWriter, r *http.Requ return } locale := store.LocaleFromContext(r.Context()) - grantID, err := uuid.Parse(r.PathValue("grantId")) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "grant")}) + g, binaryID, ok := h.getGrantForBinary(w, r, locale) + if !ok { return } - if err := h.grants.Delete(r.Context(), grantID); err != nil { - slog.Error("secure_cli_grants.delete", "grant_id", grantID, "error", err) + if err := h.grants.Delete(r.Context(), g.ID); err != nil { + slog.Error("secure_cli_grants.delete", "grant_id", g.ID, "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "delete grant")}) return } - h.emitCacheInvalidate(r.PathValue("id")) + h.emitCacheInvalidate(binaryID.String()) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } @@ -407,26 +447,9 @@ func (h *SecureCLIGrantHandler) handleRevealEnv(w http.ResponseWriter, r *http.R return } - grantID, err := uuid.Parse(r.PathValue("grantId")) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "grant")}) - return - } - binaryID, err := uuid.Parse(r.PathValue("id")) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "binary")}) - return - } - - // store.Get enforces tenant_id = $2 filter (non-cross-tenant context). - g, err := h.grants.Get(ctx, grantID) - if err != nil { - writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "grant", grantID.String())}) - return - } - // Enforce URL parent-child hierarchy: grant must belong to binaryID in path. - if g.BinaryID != binaryID { - writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "grant", grantID.String())}) + // store.Get enforces tenant_id filter; helper also enforces URL parent-child hierarchy. + g, binaryID, ok := h.getGrantForBinary(w, r, locale) + if !ok { return } @@ -438,7 +461,7 @@ func (h *SecureCLIGrantHandler) handleRevealEnv(w http.ResponseWriter, r *http.R slog.Info("audit.cli_credential.env.reveal", "caller_id", callerID, "tenant_id", tenantID, - "grant_id", grantID, + "grant_id", g.ID, "binary_id", binaryID, "reason", "reveal-env", "ts", time.Now().UTC(), @@ -455,7 +478,7 @@ func (h *SecureCLIGrantHandler) handleRevealEnv(w http.ResponseWriter, r *http.R } var envVars map[string]string if err := json.Unmarshal(g.EncryptedEnv, &envVars); err != nil { - slog.Error("secure_cli_grants.reveal.parse", "grant_id", grantID, "error", err) + slog.Error("secure_cli_grants.reveal.parse", "grant_id", g.ID, "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "parse grant env")}) return } diff --git a/internal/http/secure_cli_agent_grants_test.go b/internal/http/secure_cli_agent_grants_test.go new file mode 100644 index 00000000..fd77450e --- /dev/null +++ b/internal/http/secure_cli_agent_grants_test.go @@ -0,0 +1,204 @@ +package http + +import ( + "context" + "database/sql" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type fakeSecureCLIGrantStore struct { + binaries map[uuid.UUID]bool + agents map[uuid.UUID]bool + grants map[uuid.UUID]*store.SecureCLIAgentGrant + + createCalled bool + updateCalled bool + deleteCalled bool +} + +func (s *fakeSecureCLIGrantStore) BinaryExists(_ context.Context, id uuid.UUID) (bool, error) { + return s.binaries[id], nil +} + +func (s *fakeSecureCLIGrantStore) AgentExists(_ context.Context, id uuid.UUID) (bool, error) { + return s.agents[id], nil +} + +func (s *fakeSecureCLIGrantStore) Create(_ context.Context, g *store.SecureCLIAgentGrant) error { + s.createCalled = true + if g.ID == uuid.Nil { + g.ID = store.GenNewID() + } + s.grants[g.ID] = g + return nil +} + +func (s *fakeSecureCLIGrantStore) Get(_ context.Context, id uuid.UUID) (*store.SecureCLIAgentGrant, error) { + if g := s.grants[id]; g != nil { + cp := *g + return &cp, nil + } + return nil, sql.ErrNoRows +} + +func (s *fakeSecureCLIGrantStore) Update(context.Context, uuid.UUID, map[string]any) error { + s.updateCalled = true + return nil +} + +func (s *fakeSecureCLIGrantStore) Delete(context.Context, uuid.UUID) error { + s.deleteCalled = true + return nil +} + +func (s *fakeSecureCLIGrantStore) ListByBinary(context.Context, uuid.UUID) ([]store.SecureCLIAgentGrant, error) { + return nil, nil +} + +func (s *fakeSecureCLIGrantStore) ListByAgent(context.Context, uuid.UUID) ([]store.SecureCLIAgentGrant, error) { + return nil, nil +} + +func (s *fakeSecureCLIGrantStore) UpdateGrantEnv(context.Context, uuid.UUID, []byte) error { + s.updateCalled = true + return nil +} + +func requestWithGrantPath(method string, body io.Reader, binaryID, grantID uuid.UUID) (*httptest.ResponseRecorder, *http.Request) { + req := httptest.NewRequest(method, "/v1/cli-credentials/"+binaryID.String()+"/agent-grants/"+grantID.String(), body) + req.SetPathValue("id", binaryID.String()) + req.SetPathValue("grantId", grantID.String()) + ctx := store.WithTenantID(req.Context(), uuid.MustParse("0193a5b0-7000-7000-8000-000000000002")) + ctx = store.WithRole(ctx, store.RoleOwner) + ctx = store.WithUserID(ctx, "admin@example.com") + return httptest.NewRecorder(), req.WithContext(ctx) +} + +func requestWithBinaryPath(body io.Reader, binaryID uuid.UUID) (*httptest.ResponseRecorder, *http.Request) { + req := httptest.NewRequest(http.MethodPost, "/v1/cli-credentials/"+binaryID.String()+"/agent-grants", body) + req.SetPathValue("id", binaryID.String()) + ctx := store.WithTenantID(req.Context(), uuid.MustParse("0193a5b0-7000-7000-8000-000000000002")) + ctx = store.WithRole(ctx, store.RoleOwner) + ctx = store.WithUserID(ctx, "admin@example.com") + return httptest.NewRecorder(), req.WithContext(ctx) +} + +func TestSecureCLIGrantNestedRoutesRejectWrongBinaryParent(t *testing.T) { + realBinaryID := uuid.New() + pathBinaryID := uuid.New() + grantID := uuid.New() + fake := &fakeSecureCLIGrantStore{ + grants: map[uuid.UUID]*store.SecureCLIAgentGrant{ + grantID: { + BaseModel: store.BaseModel{ID: grantID}, + BinaryID: realBinaryID, + AgentID: uuid.New(), + Enabled: true, + EncryptedEnv: []byte(`{"TOKEN":"value"}`), + }, + }, + } + h := NewSecureCLIGrantHandler(fake, nil, nil) + + tests := []struct { + name string + method string + body string + call func(http.ResponseWriter, *http.Request) + }{ + {name: "get", method: http.MethodGet, call: h.handleGet}, + {name: "update", method: http.MethodPut, body: `{"enabled":false}`, call: h.handleUpdate}, + {name: "delete", method: http.MethodDelete, call: h.handleDelete}, + {name: "reveal", method: http.MethodPost, call: h.handleRevealEnv}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake.updateCalled = false + fake.deleteCalled = false + rr, req := requestWithGrantPath(tt.method, strings.NewReader(tt.body), pathBinaryID, grantID) + tt.call(rr, req) + if rr.Code != http.StatusNotFound { + t.Fatalf("expected 404 for wrong binary parent, got %d body=%s", rr.Code, rr.Body.String()) + } + if fake.updateCalled { + t.Fatal("wrong-parent request must not update grant or env") + } + if fake.deleteCalled { + t.Fatal("wrong-parent request must not delete grant") + } + }) + } +} + +func TestSecureCLIGrantCreateValidatesBinaryAndAgentScope(t *testing.T) { + binaryID := uuid.New() + agentID := uuid.New() + + tests := []struct { + name string + binaryOK bool + agentOK bool + wantStatus int + wantCreate bool + }{ + {name: "missing binary", binaryOK: false, agentOK: true, wantStatus: http.StatusNotFound}, + {name: "missing agent", binaryOK: true, agentOK: false, wantStatus: http.StatusNotFound}, + {name: "valid scope", binaryOK: true, agentOK: true, wantStatus: http.StatusCreated, wantCreate: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake := &fakeSecureCLIGrantStore{ + binaries: map[uuid.UUID]bool{binaryID: tt.binaryOK}, + agents: map[uuid.UUID]bool{agentID: tt.agentOK}, + grants: map[uuid.UUID]*store.SecureCLIAgentGrant{}, + } + h := NewSecureCLIGrantHandler(fake, nil, nil) + rr, req := requestWithBinaryPath(strings.NewReader(`{"agent_id":"`+agentID.String()+`","enabled":true}`), binaryID) + + h.handleCreate(rr, req) + + if rr.Code != tt.wantStatus { + t.Fatalf("expected status %d, got %d body=%s", tt.wantStatus, rr.Code, rr.Body.String()) + } + if fake.createCalled != tt.wantCreate { + t.Fatalf("createCalled=%v, want %v", fake.createCalled, tt.wantCreate) + } + }) + } +} + +func TestSecureCLIGrantUpdateRejectsInvalidEnvVarsBeforeScalarUpdate(t *testing.T) { + binaryID := uuid.New() + grantID := uuid.New() + fake := &fakeSecureCLIGrantStore{ + grants: map[uuid.UUID]*store.SecureCLIAgentGrant{ + grantID: { + BaseModel: store.BaseModel{ID: grantID}, + BinaryID: binaryID, + AgentID: uuid.New(), + Enabled: true, + }, + }, + } + h := NewSecureCLIGrantHandler(fake, nil, nil) + rr, req := requestWithGrantPath(http.MethodPut, strings.NewReader(`{"enabled":false,"env_vars":123}`), binaryID, grantID) + + h.handleUpdate(rr, req) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d body=%s", rr.Code, rr.Body.String()) + } + if fake.updateCalled { + t.Fatal("invalid env_vars request must not persist scalar grant updates") + } +} diff --git a/internal/store/pg/secure_cli_agent_grants.go b/internal/store/pg/secure_cli_agent_grants.go index db448acc..6865e9f7 100644 --- a/internal/store/pg/secure_cli_agent_grants.go +++ b/internal/store/pg/secure_cli_agent_grants.go @@ -26,6 +26,42 @@ func NewPGSecureCLIAgentGrantStore(db *sql.DB, encKey string) *PGSecureCLIAgentG const grantSelectCols = `id, binary_id, agent_id, deny_args, deny_verbose, timeout_seconds, tips, enabled, encrypted_env, created_at, updated_at` +func (s *PGSecureCLIAgentGrantStore) BinaryExists(ctx context.Context, binaryID uuid.UUID) (bool, error) { + query := `SELECT EXISTS(SELECT 1 FROM secure_cli_binaries WHERE id = $1` + args := []any{binaryID} + if !store.IsCrossTenant(ctx) { + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + return false, nil + } + query += ` AND tenant_id = $2` + args = append(args, tid) + } + query += `)` + + var exists bool + err := s.db.QueryRowContext(ctx, query, args...).Scan(&exists) + return exists, err +} + +func (s *PGSecureCLIAgentGrantStore) AgentExists(ctx context.Context, agentID uuid.UUID) (bool, error) { + query := `SELECT EXISTS(SELECT 1 FROM agents WHERE id = $1 AND deleted_at IS NULL` + args := []any{agentID} + if !store.IsCrossTenant(ctx) { + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + return false, nil + } + query += ` AND tenant_id = $2` + args = append(args, tid) + } + query += `)` + + var exists bool + err := s.db.QueryRowContext(ctx, query, args...).Scan(&exists) + return exists, err +} + func (s *PGSecureCLIAgentGrantStore) Create(ctx context.Context, g *store.SecureCLIAgentGrant) error { if g.ID == uuid.Nil { g.ID = store.GenNewID() diff --git a/internal/store/secure_cli_store.go b/internal/store/secure_cli_store.go index dffa7fec..2c8f117c 100644 --- a/internal/store/secure_cli_store.go +++ b/internal/store/secure_cli_store.go @@ -26,11 +26,11 @@ type SecureCLIBinary struct { BinaryName string `json:"binary_name" db:"binary_name"` BinaryPath *string `json:"binary_path,omitempty" db:"binary_path"` Description string `json:"description" db:"description"` - EncryptedEnv []byte `json:"-" db:"encrypted_env"` // AES-256-GCM encrypted JSON — never serialized to API + EncryptedEnv []byte `json:"-" db:"encrypted_env"` // AES-256-GCM encrypted JSON — never serialized to API DenyArgs json.RawMessage `json:"deny_args" db:"deny_args"` // regex patterns for blocked subcommands - DenyVerbose json.RawMessage `json:"deny_verbose" db:"deny_verbose"` // blocked verbose/debug flags + DenyVerbose json.RawMessage `json:"deny_verbose" db:"deny_verbose"` // blocked verbose/debug flags TimeoutSeconds int `json:"timeout_seconds" db:"timeout_seconds"` - Tips string `json:"tips" db:"tips"` // hint injected into TOOLS.md context + Tips string `json:"tips" db:"tips"` // hint injected into TOOLS.md context IsGlobal bool `json:"is_global" db:"is_global"` Enabled bool `json:"enabled" db:"enabled"` CreatedBy string `json:"created_by" db:"created_by"` @@ -67,12 +67,12 @@ func (b *SecureCLIBinary) MergeGrantOverrides(g *SecureCLIAgentGrant) { // SecureCLIUserCredential holds per-user encrypted env overrides for a binary. type SecureCLIUserCredential struct { - ID uuid.UUID `json:"id" db:"id"` - BinaryID uuid.UUID `json:"binary_id" db:"binary_id"` - UserID string `json:"user_id" db:"user_id"` - Metadata json.RawMessage `json:"metadata,omitempty" db:"metadata"` - CreatedAt string `json:"created_at" db:"created_at"` - UpdatedAt string `json:"updated_at" db:"updated_at"` + ID uuid.UUID `json:"id" db:"id"` + BinaryID uuid.UUID `json:"binary_id" db:"binary_id"` + UserID string `json:"user_id" db:"user_id"` + Metadata json.RawMessage `json:"metadata,omitempty" db:"metadata"` + CreatedAt string `json:"created_at" db:"created_at"` + UpdatedAt string `json:"updated_at" db:"updated_at"` // EncryptedEnv is decrypted JSON — never serialized to API. EncryptedEnv []byte `json:"-" db:"encrypted_env"` } @@ -89,13 +89,13 @@ type SecureCLIAgentGrant struct { Enabled bool `json:"enabled" db:"enabled"` // EncryptedEnv holds per-grant AES-256-GCM encrypted env vars. NULL means no override. // Never serialized to API — HTTP layer exposes env_keys + env_set only. - EncryptedEnv []byte `json:"-" db:"encrypted_env"` + EncryptedEnv []byte `json:"-" db:"encrypted_env"` // EnvKeys is populated by HTTP handlers only (sorted key names, no values). Not a DB column. - EnvKeys []string `json:"env_keys,omitempty" db:"-"` + EnvKeys []string `json:"env_keys,omitempty" db:"-"` // EnvSet indicates whether this grant has an env override. Not a DB column. - EnvSet bool `json:"env_set" db:"-"` - CreatedAt time.Time `json:"created_at" db:"created_at"` - UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + EnvSet bool `json:"env_set" db:"-"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } // SecureCLIStore manages secure CLI binary credential configurations. @@ -137,6 +137,8 @@ type SecureCLIStore interface { // SecureCLIAgentGrantStore manages per-agent grants for secure CLI binaries. type SecureCLIAgentGrantStore interface { + BinaryExists(ctx context.Context, binaryID uuid.UUID) (bool, error) + AgentExists(ctx context.Context, agentID uuid.UUID) (bool, error) Create(ctx context.Context, g *SecureCLIAgentGrant) error Get(ctx context.Context, id uuid.UUID) (*SecureCLIAgentGrant, error) Update(ctx context.Context, id uuid.UUID, updates map[string]any) error diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index 3c96c4e2..cf126924 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -887,6 +887,15 @@ func EnsureSchema(db *sql.DB) error { if !ok { return fmt.Errorf("sqlite: missing migration for version %d → %d", v, v+1) } + if tableName, columnName, ok := idempotentColumnMigration(v); ok { + hasColumn, err := sqliteColumnExists(db, tableName, columnName) + if err != nil { + return fmt.Errorf("inspect %s.%s: %w", tableName, columnName, err) + } + if hasColumn { + patch = `SELECT 1;` + } + } // Migrations that rebuild a table referenced by another table's FK // require foreign_keys=OFF per SQLite altertable §7. The pragma is // a no-op inside a transaction, so toggle it around BEGIN/COMMIT. @@ -953,6 +962,44 @@ func EnsureSchema(db *sql.DB) error { return seedMasterTenant(db) } +func idempotentColumnMigration(version int) (string, string, bool) { + switch version { + case 26: + return "secure_cli_agent_grants", "encrypted_env", true + case 28: + return "webhook_calls", "lease_token", true + case 29: + return "webhooks", "encrypted_secret", true + case 33: + return "agents", "model_fallback", true + default: + return "", "", false + } +} + +func sqliteColumnExists(db *sql.DB, tableName, columnName string) (bool, error) { + rows, err := db.Query("PRAGMA table_info(" + tableName + ")") + if err != nil { + return false, err + } + defer rows.Close() + + for rows.Next() { + var cid int + var name, colType string + var notNull int + var defaultValue any + var pk int + if err := rows.Scan(&cid, &name, &colType, ¬Null, &defaultValue, &pk); err != nil { + return false, err + } + if name == columnName { + return true, nil + } + } + return false, rows.Err() +} + // seedMasterTenant ensures the master tenant row exists (idempotent). func seedMasterTenant(db *sql.DB) error { _, err := db.Exec( diff --git a/internal/store/sqlitestore/secure-cli-agent-grants.go b/internal/store/sqlitestore/secure-cli-agent-grants.go index 351be864..6609e631 100644 --- a/internal/store/sqlitestore/secure-cli-agent-grants.go +++ b/internal/store/sqlitestore/secure-cli-agent-grants.go @@ -6,8 +6,8 @@ import ( "context" "database/sql" "encoding/json" - "log/slog" "fmt" + "log/slog" "time" "github.com/google/uuid" @@ -29,6 +29,42 @@ func NewSQLiteSecureCLIAgentGrantStore(db *sql.DB, encKey string) *SQLiteSecureC const grantSelectCols = `id, binary_id, agent_id, deny_args, deny_verbose, timeout_seconds, tips, enabled, encrypted_env, created_at, updated_at` +func (s *SQLiteSecureCLIAgentGrantStore) BinaryExists(ctx context.Context, binaryID uuid.UUID) (bool, error) { + query := `SELECT EXISTS(SELECT 1 FROM secure_cli_binaries WHERE id = ?` + args := []any{binaryID} + if !store.IsCrossTenant(ctx) { + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + return false, nil + } + query += ` AND tenant_id = ?` + args = append(args, tid) + } + query += `)` + + var exists bool + err := s.db.QueryRowContext(ctx, query, args...).Scan(&exists) + return exists, err +} + +func (s *SQLiteSecureCLIAgentGrantStore) AgentExists(ctx context.Context, agentID uuid.UUID) (bool, error) { + query := `SELECT EXISTS(SELECT 1 FROM agents WHERE id = ? AND deleted_at IS NULL` + args := []any{agentID} + if !store.IsCrossTenant(ctx) { + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + return false, nil + } + query += ` AND tenant_id = ?` + args = append(args, tid) + } + query += `)` + + var exists bool + err := s.db.QueryRowContext(ctx, query, args...).Scan(&exists) + return exists, err +} + func (s *SQLiteSecureCLIAgentGrantStore) Create(ctx context.Context, g *store.SecureCLIAgentGrant) error { if g.ID == uuid.Nil { g.ID = store.GenNewID() diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 55fe295c..523e7832 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -373,20 +373,11 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL return credentialedDenyError(binary, args, p) } - // Step 4: Decrypt env vars from store (already decrypted by store layer) - envMap := make(map[string]string) - if len(cred.EncryptedEnv) > 0 { - if err := json.Unmarshal(cred.EncryptedEnv, &envMap); err != nil { - return ErrorResult(fmt.Sprintf("credentialed exec: invalid env JSON for %q: %v", binary, err)) - } - } - - // Step 4b: Merge per-user env overrides (user takes priority over base) - if len(cred.UserEnv) > 0 { - var userEnvMap map[string]string - if err := json.Unmarshal(cred.UserEnv, &userEnvMap); err == nil { - maps.Copy(envMap, userEnvMap) - } + // Step 4: Decrypt env vars from store (already decrypted by store layer). + // Per-user env overrides take priority over binary/grant env. + envMap, err := mergeCredentialedEnv(cred) + if err != nil { + return ErrorResult(fmt.Sprintf("credentialed exec: invalid env JSON for %q: %v", binary, err)) } // Step 5: Register credential values for output scrubbing @@ -407,6 +398,26 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL return t.executeCredentialedHost(ctx, absPath, args, cwd, envMap, timeout) } +func mergeCredentialedEnv(cred *store.SecureCLIBinary) (map[string]string, error) { + envMap := make(map[string]string) + if cred == nil { + return envMap, nil + } + if len(cred.EncryptedEnv) > 0 { + if err := json.Unmarshal(cred.EncryptedEnv, &envMap); err != nil { + return nil, err + } + } + if len(cred.UserEnv) > 0 { + var userEnvMap map[string]string + if err := json.Unmarshal(cred.UserEnv, &userEnvMap); err != nil { + return nil, err + } + maps.Copy(envMap, userEnvMap) + } + return envMap, nil +} + // executeCredentialedHost runs a credentialed command directly on the host. // Uses exec.Command (no shell) with credentials as env vars. // ctx cancellation triggers SIGTERM → 3s grace → SIGKILL via process-group helpers. diff --git a/internal/tools/credentialed_exec_env_test.go b/internal/tools/credentialed_exec_env_test.go new file mode 100644 index 00000000..e164a8ae --- /dev/null +++ b/internal/tools/credentialed_exec_env_test.go @@ -0,0 +1,45 @@ +package tools + +import ( + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +func TestMergeCredentialedEnvPerUserOverridesGrantEnv(t *testing.T) { + binary := &store.SecureCLIBinary{ + EncryptedEnv: []byte(`{"SHARED_KEY":"binary","BINARY_ONLY":"base"}`), + } + binary.MergeGrantOverrides(&store.SecureCLIAgentGrant{ + EncryptedEnv: []byte(`{"SHARED_KEY":"grant","GRANT_ONLY":"agent"}`), + }) + binary.UserEnv = []byte(`{"SHARED_KEY":"user","USER_ONLY":"personal"}`) + + env, err := mergeCredentialedEnv(binary) + if err != nil { + t.Fatalf("mergeCredentialedEnv returned error: %v", err) + } + + if got := env["SHARED_KEY"]; got != "user" { + t.Fatalf("expected per-user env to win for duplicate key, got %q", got) + } + if got := env["GRANT_ONLY"]; got != "agent" { + t.Fatalf("expected grant env key to remain, got %q", got) + } + if got := env["USER_ONLY"]; got != "personal" { + t.Fatalf("expected per-user env key to remain, got %q", got) + } + if _, ok := env["BINARY_ONLY"]; ok { + t.Fatal("expected agent grant env to replace binary default env") + } +} + +func TestMergeCredentialedEnvFailsClosedOnInvalidUserEnv(t *testing.T) { + _, err := mergeCredentialedEnv(&store.SecureCLIBinary{ + EncryptedEnv: []byte(`{"SHARED_KEY":"grant"}`), + UserEnv: []byte(`{broken json`), + }) + if err == nil { + t.Fatal("expected invalid per-user env JSON to fail closed") + } +} diff --git a/ui/web/src/components/layout/sidebar.tsx b/ui/web/src/components/layout/sidebar.tsx index 100224ef..0ef43c30 100644 --- a/ui/web/src/components/layout/sidebar.tsx +++ b/ui/web/src/components/layout/sidebar.tsx @@ -136,7 +136,6 @@ export function Sidebar({ collapsed, onNavItemClick }: SidebarProps) { )} - {isOwner && ( diff --git a/ui/web/src/pages/cli-credentials/__tests__/cli-credential-grants-dialog-helpers.test.ts b/ui/web/src/pages/cli-credentials/__tests__/cli-credential-grants-dialog-helpers.test.ts new file mode 100644 index 00000000..7f290f11 --- /dev/null +++ b/ui/web/src/pages/cli-credentials/__tests__/cli-credential-grants-dialog-helpers.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "vitest"; +import { + buildEnvVarsPayload, + EMPTY_ENV_STATE, + envStateFromGrant, +} from "../cli-credential-grants-dialog-helpers"; +import type { CLIAgentGrant } from "../hooks/use-cli-credentials"; + +describe("cli credential grant env helpers", () => { + it("omits env_vars when existing masked values are not revealed", () => { + const payload = buildEnvVarsPayload( + { overrideEnabled: true, entries: [{ key: "TOKEN", value: "", masked: true }] }, + true, + ); + expect(payload).toBeUndefined(); + }); + + it("serializes only visible env entries", () => { + const payload = buildEnvVarsPayload( + { + overrideEnabled: true, + entries: [ + { key: " CLI_ENV ", value: "agent-value", masked: false }, + { key: "", value: "ignored", masked: false }, + { key: "MASKED", value: "", masked: true }, + ], + }, + false, + ); + expect(payload).toEqual({ CLI_ENV: "agent-value" }); + }); + + it("clears existing env override when override is disabled", () => { + expect(buildEnvVarsPayload(EMPTY_ENV_STATE, true)).toBeNull(); + expect(buildEnvVarsPayload(EMPTY_ENV_STATE, false)).toBeUndefined(); + }); + + it("derives masked state from grant env metadata without values", () => { + const state = envStateFromGrant({ + env_set: true, + env_keys: ["API_KEY", "TOKEN"], + } as CLIAgentGrant); + + expect(state).toEqual({ + overrideEnabled: true, + entries: [ + { key: "API_KEY", value: "", masked: true }, + { key: "TOKEN", value: "", masked: true }, + ], + }); + }); +}); diff --git a/ui/web/src/pages/packages/__tests__/cli-credentials-routing.test.ts b/ui/web/src/pages/packages/__tests__/cli-credentials-routing.test.ts new file mode 100644 index 00000000..a4a0b8ca --- /dev/null +++ b/ui/web/src/pages/packages/__tests__/cli-credentials-routing.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +function source(path: string): string { + return readFileSync(resolve(process.cwd(), path), "utf8"); +} + +describe("CLI Credentials package routing", () => { + it("keeps CLI Credentials inside Packages and out of the left sidebar", () => { + const sidebar = source("src/components/layout/sidebar.tsx"); + const packagesPage = source("src/pages/packages/packages-page.tsx"); + + expect(sidebar).not.toContain("ROUTES.CLI_CREDENTIALS"); + expect(sidebar).not.toContain("nav.cliCredentials"); + expect(packagesPage).toContain('"cli-credentials"'); + expect(packagesPage).toContain("CliCredentialsTab"); + }); + + it("keeps the legacy /cli-credentials route as a redirect to the Packages tab", () => { + const routes = source("src/routes.tsx"); + + expect(routes).toContain("ROUTES.CLI_CREDENTIALS"); + expect(routes).toContain("/packages?tab=cli-credentials"); + }); +});