diff --git a/docs/03-tools-system.md b/docs/03-tools-system.md index c7940b6d..d1b3dd8b 100644 --- a/docs/03-tools-system.md +++ b/docs/03-tools-system.md @@ -372,6 +372,13 @@ Custom tools are shell-based tools defined at runtime via the HTTP API — no re | `env` | no | Encrypted environment variables injected at runtime | | `enabled` | no | Toggle without deleting (default true) | +Credentialed CLI env entries support two API/UI kinds: + +- `sensitive` (default): encrypted at rest, masked in normal API responses, replace-only in UI, and flattened only at credential injection time. +- `value`: encrypted at rest but visible to authorized admins in API/UI for non-secret settings such as public URLs, domains, limits, regions, and feature flags. + +Legacy env JSON like `{"TOKEN":"..."}` is still accepted and treated as `sensitive`. + **Execution:** Template placeholders are rendered with shell-escaped argument values, then run via `sh -c`. The same deny-pattern check as the `exec` tool applies — no reverse shells, no `curl | sh`, etc. **Scope:** diff --git a/docs/09-security.md b/docs/09-security.md index 124e4fb8..5fcdc693 100644 --- a/docs/09-security.md +++ b/docs/09-security.md @@ -221,11 +221,14 @@ AES-256-GCM encryption for secrets stored in PostgreSQL. Key provided via `GOCLA | LLM provider API keys | `llm_providers` | `api_key` | | MCP server API keys | `mcp_servers` | `api_key` | | Custom tool env vars | `custom_tools` | `env` | +| Credentialed CLI env vars | `secure_cli_binaries`, `secure_cli_agent_grants`, `secure_cli_user_credentials` | `encrypted_env` | **Format**: `"aes-gcm:" + base64(12-byte nonce + ciphertext + GCM tag)` Backward compatible: values without the `aes-gcm:` prefix are returned as plaintext (for migration from unencrypted data). +Credentialed CLI env entries have a separate visibility kind inside the encrypted JSON blob when `GOCLAW_ENCRYPTION_KEY` is configured. `sensitive` entries are masked in normal API/UI responses and never returned raw except through the explicit audited grant reveal flow. `value` entries use the same at-rest storage path but are returned to authorized admins for operational review. + --- ## 4. Rate Limiting -- Gateway + Tool diff --git a/docs/project-changelog.md b/docs/project-changelog.md index e795c70d..d32a8800 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,23 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-24 +### CLI environment variable visibility + +**Features** + +- Added `sensitive` and `value` kinds for secure CLI environment variables across binary defaults, agent grant overrides, and user overrides. +- Plain value entries are visible to authorized admins for operational config review, while sensitive entries remain masked and replace-only. + +**Fixes** + +- Stopped per-user credential reads from returning legacy sensitive env values raw. +- Kept legacy `{"KEY":"value"}` env blobs backward-compatible by treating them as sensitive. + +**Tests** + +- Added backend regression coverage for env kind parsing, sanitized API responses, runtime flattening, and invalid kind rejection. +- Verified Web UI build after adding env-kind controls and warnings. + ### Command keyword allowlist **Features** diff --git a/internal/http/secure_cli.go b/internal/http/secure_cli.go index 5aa05a5c..ee4d0ad8 100644 --- a/internal/http/secure_cli.go +++ b/internal/http/secure_cli.go @@ -7,7 +7,6 @@ import ( "net/http" "os/exec" "regexp" - "sort" "strings" "github.com/google/uuid" @@ -70,68 +69,13 @@ func (h *SecureCLIHandler) emitCacheInvalidate(key string) { // envKeysFromDecryptedJSON returns sorted env variable names from plaintext env JSON (decrypted blob). func envKeysFromDecryptedJSON(env []byte) []string { - empty := []string{} - if len(env) == 0 { - return empty - } - var m map[string]any - if err := json.Unmarshal(env, &m); err != nil { - return empty - } - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) - return keys + return store.SecureCLIEnvKeys(env) } -// mergeSecureCLIEnv merges incoming env from the UI with existing stored env. -// Incoming defines the full set of keys shown in the form: keys omitted were removed. -// Empty string means "keep existing value" for that key when it already exists. -func mergeSecureCLIEnv(existingJSON []byte, incoming map[string]any) (map[string]string, error) { - existing := map[string]string{} - if len(existingJSON) > 0 { - if err := json.Unmarshal(existingJSON, &existing); err != nil { - return nil, fmt.Errorf("parse existing env: %w", err) - } - } - out := make(map[string]string) - for k, v := range incoming { - if k == "" { - continue - } - sv, err := envValueAsString(v) - if err != nil { - return nil, fmt.Errorf("invalid environment variable value") - } - if sv != "" { - out[k] = sv - continue - } - if ev, ok := existing[k]; ok { - out[k] = ev - } - } - return out, nil -} - -func envValueAsString(v any) (string, error) { - switch t := v.(type) { - case string: - return t, nil - case float64: - return fmt.Sprint(t), nil - case bool: - if t { - return "true", nil - } - return "false", nil - case nil: - return "", nil - default: - return "", fmt.Errorf("value must be a string") - } +func populateBinaryEnvResponse(b *store.SecureCLIBinary) { + b.EnvKeys = store.SecureCLIEnvKeys(b.EncryptedEnv) + b.Env = store.SanitizeSecureCLIEnvJSON(b.EncryptedEnv) + b.EncryptedEnv = nil } func (h *SecureCLIHandler) handleList(w http.ResponseWriter, r *http.Request) { @@ -142,27 +86,26 @@ func (h *SecureCLIHandler) handleList(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgFailedToList, "CLI credentials")}) return } - // Never send env values; only variable names for editing. + // Sensitive env values stay masked; value-kind entries are returned for editing. for i := range result { - result[i].EnvKeys = envKeysFromDecryptedJSON(result[i].EncryptedEnv) - result[i].EncryptedEnv = nil + populateBinaryEnvResponse(&result[i]) } writeJSON(w, http.StatusOK, map[string]any{"items": result}) } // secureCLICreateRequest supports both preset-based and custom creation. type secureCLICreateRequest struct { - Preset string `json:"preset,omitempty"` // auto-fill from preset - BinaryName string `json:"binary_name"` - BinaryPath *string `json:"binary_path,omitempty"` - Description string `json:"description"` - Env map[string]string `json:"env"` // plaintext env vars (encrypted by store) - DenyArgs json.RawMessage `json:"deny_args,omitempty"` - DenyVerbose json.RawMessage `json:"deny_verbose,omitempty"` - TimeoutSeconds int `json:"timeout_seconds,omitempty"` - Tips string `json:"tips,omitempty"` - IsGlobal *bool `json:"is_global,omitempty"` - Enabled bool `json:"enabled"` + Preset string `json:"preset,omitempty"` // auto-fill from preset + BinaryName string `json:"binary_name"` + BinaryPath *string `json:"binary_path,omitempty"` + Description string `json:"description"` + Env json.RawMessage `json:"env"` // plaintext env vars or env entry objects (encrypted by store) + DenyArgs json.RawMessage `json:"deny_args,omitempty"` + DenyVerbose json.RawMessage `json:"deny_verbose,omitempty"` + TimeoutSeconds int `json:"timeout_seconds,omitempty"` + Tips string `json:"tips,omitempty"` + IsGlobal *bool `json:"is_global,omitempty"` + Enabled bool `json:"enabled"` } func (h *SecureCLIHandler) handleCreate(w http.ResponseWriter, r *http.Request) { @@ -209,10 +152,14 @@ func (h *SecureCLIHandler) handleCreate(w http.ResponseWriter, r *http.Request) return } - // Serialize env as JSON bytes (store layer encrypts) - envJSON, err := json.Marshal(req.Env) + envEntries, err := store.ParseSecureCLIEnv(req.Env) if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid env"}) + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error())}) + return + } + envJSON, err := store.SerializeSecureCLIEnv(envEntries) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error())}) return } @@ -240,7 +187,7 @@ func (h *SecureCLIHandler) handleCreate(w http.ResponseWriter, r *http.Request) } tools.ResetCredentialScrubValues() // clear stale scrub values - b.EncryptedEnv = nil // don't return credentials + populateBinaryEnvResponse(b) emitAudit(h.msgBus, r, "secure_cli.created", "secure_cli", b.ID.String()) h.emitCacheInvalidate(b.ID.String()) writeJSON(w, http.StatusCreated, b) @@ -260,8 +207,7 @@ func (h *SecureCLIHandler) handleGet(w http.ResponseWriter, r *http.Request) { return } - b.EnvKeys = envKeysFromDecryptedJSON(b.EncryptedEnv) - b.EncryptedEnv = nil // don't expose credential values + populateBinaryEnvResponse(b) writeJSON(w, http.StatusOK, b) } @@ -293,25 +239,23 @@ func (h *SecureCLIHandler) handleUpdate(w http.ResponseWriter, r *http.Request) // If env is updated, merge with stored env so empty values mean "keep existing secret". if envVal, ok := updates["env"]; ok { - if envMap, isMap := envVal.(map[string]any); isMap { - cur, err := h.store.Get(r.Context(), id) - if err != nil { - writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "credential", id.String())}) - return - } - merged, err := mergeSecureCLIEnv(cur.EncryptedEnv, envMap) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) - return - } - envJSON, err := json.Marshal(merged) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid env"}) - return - } - updates["encrypted_env"] = string(envJSON) - delete(updates, "env") + envJSON, err := json.Marshal(envVal) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error())}) + return } + cur, err := h.store.Get(r.Context(), id) + if err != nil { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "credential", id.String())}) + return + } + merged, err := store.MergeSecureCLIEnv(cur.EncryptedEnv, envJSON) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error())}) + return + } + updates["encrypted_env"] = string(merged) + delete(updates, "env") } if err := h.store.Update(r.Context(), id, updates); err != nil { diff --git a/internal/http/secure_cli_agent_grants.go b/internal/http/secure_cli_agent_grants.go index 11f87289..342a9b2e 100644 --- a/internal/http/secure_cli_agent_grants.go +++ b/internal/http/secure_cli_agent_grants.go @@ -68,7 +68,7 @@ func (h *SecureCLIGrantHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("GET /v1/cli-credentials/{id}/agent-grants/{grantId}", auth(h.handleGet)) mux.HandleFunc("PUT /v1/cli-credentials/{id}/agent-grants/{grantId}", auth(h.handleUpdate)) mux.HandleFunc("DELETE /v1/cli-credentials/{id}/agent-grants/{grantId}", auth(h.handleDelete)) - // POST (not GET) to prevent caching and satisfy CSRF semantics per Red Team C1. + // POST keeps revealed secret material out of URL/history and avoids query caching. mux.HandleFunc("POST /v1/cli-credentials/{id}/agent-grants/{grantId}/env:reveal", auth(h.handleRevealEnv)) } @@ -76,45 +76,41 @@ func (h *SecureCLIGrantHandler) RegisterRoutes(mux *http.ServeMux) { // EnvVars is optional; plaintext values are encrypted by the store layer. // Clients MUST NOT send encrypted_env — that field is never accepted from the wire. type grantCreateRequest struct { - AgentID uuid.UUID `json:"agent_id"` - EnvVars map[string]string `json:"env_vars,omitempty"` - DenyArgs *json.RawMessage `json:"deny_args,omitempty"` - DenyVerbose *json.RawMessage `json:"deny_verbose,omitempty"` - TimeoutSeconds *int `json:"timeout_seconds,omitempty"` - Tips *string `json:"tips,omitempty"` - Enabled *bool `json:"enabled,omitempty"` + AgentID uuid.UUID `json:"agent_id"` + EnvVars json.RawMessage `json:"env_vars,omitempty"` + DenyArgs *json.RawMessage `json:"deny_args,omitempty"` + DenyVerbose *json.RawMessage `json:"deny_verbose,omitempty"` + TimeoutSeconds *int `json:"timeout_seconds,omitempty"` + Tips *string `json:"tips,omitempty"` + Enabled *bool `json:"enabled,omitempty"` } -// populateGrantEnvFields sets EnvKeys (sorted) and EnvSet from the grant's decrypted env bytes. -// Plaintext values are never exposed — only key names. +// populateGrantEnvFields sets sorted key names, env presence, and sanitized entries. func populateGrantEnvFields(g *store.SecureCLIAgentGrant) { if len(g.EncryptedEnv) == 0 { g.EnvKeys = []string{} + g.Env = nil g.EnvSet = false return } - var m map[string]any - if err := json.Unmarshal(g.EncryptedEnv, &m); err != nil { - g.EnvKeys = []string{} - g.EnvSet = false - return - } - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) + keys := store.SecureCLIEnvKeys(g.EncryptedEnv) g.EnvKeys = keys + g.Env = store.SanitizeSecureCLIEnvJSON(g.EncryptedEnv) g.EnvSet = len(keys) > 0 } // validateAndSerializeEnvVars validates env keys/values via denylist and returns serialized JSON. // Returns (nil, 400 error response written) on denial, (jsonBytes, nil) on success. // Never logs env values or keys in error paths. -func validateAndSerializeEnvVars(w http.ResponseWriter, locale string, envVars map[string]string) ([]byte, bool) { - if len(envVars) == 0 { - b, _ := json.Marshal(envVars) - return b, true +func validateAndSerializeEnvVars(w http.ResponseWriter, locale string, raw json.RawMessage) ([]byte, bool) { + envEntries, err := store.ParseSecureCLIEnv(raw) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error())}) + return nil, false + } + envVars := make(map[string]string, len(envEntries)) + for key, entry := range envEntries { + envVars[key] = entry.Value } denied, valErr := crypto.ValidateGrantEnvVars(envVars) if valErr != nil { @@ -129,7 +125,7 @@ func validateAndSerializeEnvVars(w http.ResponseWriter, locale string, envVars m }) return nil, false } - b, err := json.Marshal(envVars) + b, err := store.SerializeSecureCLIEnv(envEntries) if err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, "serialization failed")}) return nil, false @@ -249,7 +245,7 @@ func (h *SecureCLIGrantHandler) handleCreate(w http.ResponseWriter, r *http.Requ envJSON, ok := validateAndSerializeEnvVars(w, locale, req.EnvVars) if !ok { // Grant was created but env validation failed; clean it up to avoid orphan row. - // Finding #13: log rollback-delete failures for ops visibility. + // Log rollback-delete failures so operators can clean up orphan rows. if delErr := h.grants.Delete(r.Context(), g.ID); delErr != nil { slog.Error("secure_cli_grants.create.rollback_delete", "grant_id", g.ID, @@ -261,7 +257,7 @@ func (h *SecureCLIGrantHandler) handleCreate(w http.ResponseWriter, r *http.Requ } if err := h.grants.UpdateGrantEnv(r.Context(), g.ID, envJSON); err != nil { slog.Error("secure_cli_grants.create.set_env", "grant_id", g.ID, "error", err) - // Finding #13: log rollback-delete failures for ops visibility. + // Log rollback-delete failures so operators can clean up orphan rows. if delErr := h.grants.Delete(r.Context(), g.ID); delErr != nil { slog.Error("secure_cli_grants.create.rollback_delete", "grant_id", g.ID, @@ -323,7 +319,7 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ } if allowedScalar[k] { var decoded any - // Finding #3: return 400 on Unmarshal failure — silent discard means admin + // Return 400 on unmarshal failure; silent discard means admin // thinks they applied a change (e.g. enabled: "false") but the grant is unchanged. if err := json.Unmarshal(v, &decoded); err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{ @@ -335,7 +331,7 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ } } // 3-state env_vars semantics: absent=skip, null=clear, {...}=replace. - // Finding #15: {} (empty map) is treated as clear — same as null. + // 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 @@ -343,18 +339,22 @@ func (h *SecureCLIGrantHandler) handleUpdate(w http.ResponseWriter, r *http.Requ envPresent = true var envPtr *map[string]string if string(envRaw) != "null" { - var m map[string]string - if err := json.Unmarshal(envRaw, &m); err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, "env_vars must be a string map")}) + envEntries, err := store.ParseSecureCLIEnv(envRaw) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, "env_vars must be a string map or env entries")}) return } + m := make(map[string]string, len(envEntries)) + for key, entry := range envEntries { + m[key] = entry.Value + } envPtr = &m } // 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. if envPtr != nil && len(*envPtr) > 0 { - j, ok := validateAndSerializeEnvVars(w, locale, *envPtr) + j, ok := validateAndSerializeEnvVars(w, locale, envRaw) if !ok { return } @@ -430,8 +430,8 @@ func (h *SecureCLIGrantHandler) handleRevealEnv(w http.ResponseWriter, r *http.R locale := store.LocaleFromContext(ctx) // Rate limit: 10 reveals/min per authenticated caller (context UserID). - // Finding #2: require non-empty UserID from authenticated context. - // If UserID is empty, the auth middleware failed to populate it — reject rather + // Require non-empty UserID from authenticated context. + // If UserID is empty, auth middleware failed to populate it — reject rather // than fall back to a spoofable header or IP address. callerID := store.UserIDFromContext(ctx) if callerID == "" { @@ -476,8 +476,8 @@ func (h *SecureCLIGrantHandler) handleRevealEnv(w http.ResponseWriter, r *http.R writeJSON(w, http.StatusOK, map[string]any{"env_vars": map[string]string{}}) return } - var envVars map[string]string - if err := json.Unmarshal(g.EncryptedEnv, &envVars); err != nil { + envVars, err := store.FlattenSecureCLIEnv(g.EncryptedEnv) + if err != nil { 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 index fd77450e..9a4f322c 100644 --- a/internal/http/secure_cli_agent_grants_test.go +++ b/internal/http/secure_cli_agent_grants_test.go @@ -3,6 +3,7 @@ package http import ( "context" "database/sql" + "encoding/json" "io" "net/http" "net/http/httptest" @@ -59,8 +60,16 @@ func (s *fakeSecureCLIGrantStore) Delete(context.Context, uuid.UUID) error { return nil } -func (s *fakeSecureCLIGrantStore) ListByBinary(context.Context, uuid.UUID) ([]store.SecureCLIAgentGrant, error) { - return nil, nil +func (s *fakeSecureCLIGrantStore) ListByBinary(_ context.Context, binaryID uuid.UUID) ([]store.SecureCLIAgentGrant, error) { + grants := make([]store.SecureCLIAgentGrant, 0, len(s.grants)) + for _, grant := range s.grants { + if grant == nil || grant.BinaryID != binaryID { + continue + } + cp := *grant + grants = append(grants, cp) + } + return grants, nil } func (s *fakeSecureCLIGrantStore) ListByAgent(context.Context, uuid.UUID) ([]store.SecureCLIAgentGrant, error) { @@ -202,3 +211,42 @@ func TestSecureCLIGrantUpdateRejectsInvalidEnvVarsBeforeScalarUpdate(t *testing. t.Fatal("invalid env_vars request must not persist scalar grant updates") } } + +func TestSecureCLIGrantGetSanitizesMixedEnv(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, + EncryptedEnv: []byte(`{"TOKEN":"secret-token","PUBLIC_BASE_URL":{"kind":"value","value":"https://goclaw.sh"}}`), + }, + }, + } + h := NewSecureCLIGrantHandler(fake, nil, nil) + rr, req := requestWithGrantPath(http.MethodGet, nil, binaryID, grantID) + + h.handleGet(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d body=%s", rr.Code, rr.Body.String()) + } + if strings.Contains(rr.Body.String(), "secret-token") { + t.Fatalf("sensitive grant env leaked in response: %s", rr.Body.String()) + } + var got struct { + Env map[string]store.SecureCLIEnvResponseEntry `json:"env"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if !got.Env["TOKEN"].Masked || got.Env["TOKEN"].Value != nil { + t.Fatalf("TOKEN not masked: %#v", got.Env["TOKEN"]) + } + if got.Env["PUBLIC_BASE_URL"].Value == nil || *got.Env["PUBLIC_BASE_URL"].Value != "https://goclaw.sh" { + t.Fatalf("PUBLIC_BASE_URL not returned: %#v", got.Env["PUBLIC_BASE_URL"]) + } +} diff --git a/internal/http/secure_cli_env_response_test.go b/internal/http/secure_cli_env_response_test.go new file mode 100644 index 00000000..4e08bcd0 --- /dev/null +++ b/internal/http/secure_cli_env_response_test.go @@ -0,0 +1,129 @@ +package http + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type fakeSecureCLIStore struct { + binary *store.SecureCLIBinary + user *store.SecureCLIUserCredential +} + +func (s *fakeSecureCLIStore) Create(context.Context, *store.SecureCLIBinary) error { return nil } +func (s *fakeSecureCLIStore) Get(context.Context, uuid.UUID) (*store.SecureCLIBinary, error) { + cp := *s.binary + return &cp, nil +} +func (s *fakeSecureCLIStore) Update(context.Context, uuid.UUID, map[string]any) error { return nil } +func (s *fakeSecureCLIStore) Delete(context.Context, uuid.UUID) error { return nil } +func (s *fakeSecureCLIStore) List(context.Context) ([]store.SecureCLIBinary, error) { + cp := *s.binary + return []store.SecureCLIBinary{cp}, nil +} +func (s *fakeSecureCLIStore) LookupByBinary(context.Context, string, *uuid.UUID, string) (*store.SecureCLIBinary, error) { + return nil, nil +} +func (s *fakeSecureCLIStore) ListEnabled(context.Context) ([]store.SecureCLIBinary, error) { + return nil, nil +} +func (s *fakeSecureCLIStore) ListForAgent(context.Context, uuid.UUID) ([]store.SecureCLIBinary, error) { + return nil, nil +} +func (s *fakeSecureCLIStore) IsRegisteredBinary(context.Context, string) (bool, error) { + return false, nil +} +func (s *fakeSecureCLIStore) GetUserCredentials(context.Context, uuid.UUID, string) (*store.SecureCLIUserCredential, error) { + cp := *s.user + return &cp, nil +} +func (s *fakeSecureCLIStore) SetUserCredentials(context.Context, uuid.UUID, string, []byte) error { + return nil +} +func (s *fakeSecureCLIStore) DeleteUserCredentials(context.Context, uuid.UUID, string) error { + return nil +} +func (s *fakeSecureCLIStore) ListUserCredentials(context.Context, uuid.UUID) ([]store.SecureCLIUserCredential, error) { + cp := *s.user + return []store.SecureCLIUserCredential{cp}, nil +} + +func TestSecureCLIGetSanitizesMixedEnv(t *testing.T) { + id := uuid.New() + h := NewSecureCLIHandler(&fakeSecureCLIStore{ + binary: &store.SecureCLIBinary{ + BaseModel: store.BaseModel{ID: id}, + BinaryName: "gh", + EncryptedEnv: []byte(`{"TOKEN":"secret-token","PUBLIC_BASE_URL":{"kind":"value","value":"https://goclaw.sh"}}`), + }, + }, nil) + req := httptest.NewRequest(http.MethodGet, "/v1/cli-credentials/"+id.String(), nil) + req.SetPathValue("id", id.String()) + rec := httptest.NewRecorder() + + h.handleGet(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "secret-token") { + t.Fatalf("sensitive env leaked in response: %s", rec.Body.String()) + } + var got struct { + Env map[string]store.SecureCLIEnvResponseEntry `json:"env"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if !got.Env["TOKEN"].Masked || got.Env["TOKEN"].Value != nil { + t.Fatalf("TOKEN not masked: %#v", got.Env["TOKEN"]) + } + if got.Env["PUBLIC_BASE_URL"].Value == nil || *got.Env["PUBLIC_BASE_URL"].Value != "https://goclaw.sh" { + t.Fatalf("value env not returned: %#v", got.Env["PUBLIC_BASE_URL"]) + } +} + +func TestSecureCLIUserCredentialsGetDoesNotReturnLegacySensitiveRaw(t *testing.T) { + binaryID := uuid.New() + h := NewSecureCLIHandler(&fakeSecureCLIStore{ + user: &store.SecureCLIUserCredential{ + ID: uuid.New(), + BinaryID: binaryID, + UserID: "user-1", + EncryptedEnv: []byte(`{"TOKEN":"secret-token","REGION":{"kind":"value","value":"asia-southeast1"}}`), + }, + }, nil) + req := httptest.NewRequest(http.MethodGet, "/v1/cli-credentials/"+binaryID.String()+"/user-credentials/user-1", nil) + req.SetPathValue("id", binaryID.String()) + req.SetPathValue("userId", "user-1") + rec := httptest.NewRecorder() + + h.handleGetUserCredentials(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "secret-token") { + t.Fatalf("legacy sensitive env leaked in response: %s", rec.Body.String()) + } + var got struct { + Env map[string]store.SecureCLIEnvResponseEntry `json:"env"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if !got.Env["TOKEN"].Masked || got.Env["TOKEN"].Value != nil { + t.Fatalf("TOKEN not masked: %#v", got.Env["TOKEN"]) + } + if got.Env["REGION"].Value == nil || *got.Env["REGION"].Value != "asia-southeast1" { + t.Fatalf("REGION not returned: %#v", got.Env["REGION"]) + } +} diff --git a/internal/http/secure_cli_user_credentials.go b/internal/http/secure_cli_user_credentials.go index 7f810594..5f089acf 100644 --- a/internal/http/secure_cli_user_credentials.go +++ b/internal/http/secure_cli_user_credentials.go @@ -25,13 +25,14 @@ func (h *SecureCLIHandler) handleListUserCredentials(w http.ResponseWriter, r *h } // Return without env values for listing (names only + timestamps) type entry struct { - ID uuid.UUID `json:"id"` - BinaryID uuid.UUID `json:"binary_id"` - UserID string `json:"user_id"` - HasEnv bool `json:"has_env"` - EnvKeys []string `json:"env_keys,omitempty"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + ID uuid.UUID `json:"id"` + BinaryID uuid.UUID `json:"binary_id"` + UserID string `json:"user_id"` + HasEnv bool `json:"has_env"` + EnvKeys []string `json:"env_keys,omitempty"` + Env map[string]store.SecureCLIEnvResponseEntry `json:"env,omitempty"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` } entries := make([]entry, 0, len(creds)) for _, c := range creds { @@ -42,6 +43,7 @@ func (h *SecureCLIHandler) handleListUserCredentials(w http.ResponseWriter, r *h UserID: c.UserID, HasEnv: len(c.EncryptedEnv) > 0, EnvKeys: envKeys, + Env: store.SanitizeSecureCLIEnvJSON(c.EncryptedEnv), CreatedAt: c.CreatedAt, UpdatedAt: c.UpdatedAt, }) @@ -73,14 +75,9 @@ func (h *SecureCLIHandler) handleGetUserCredentials(w http.ResponseWriter, r *ht return } - // Return decrypted env as JSON object (admin-only endpoint) - var envObj any - if len(cred.EncryptedEnv) > 0 { - _ = json.Unmarshal(cred.EncryptedEnv, &envObj) - } writeJSON(w, http.StatusOK, map[string]any{ "user_id": cred.UserID, - "env": envObj, + "env": store.SanitizeSecureCLIEnvJSON(cred.EncryptedEnv), }) } @@ -108,14 +105,23 @@ func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *ht return } - // Validate env is a JSON object - var envCheck map[string]string - if err := json.Unmarshal(body.Env, &envCheck); err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "env must be a JSON object with string values"}) + existing, err := h.store.GetUserCredentials(r.Context(), binaryID, userID) + if err != nil { + locale := store.LocaleFromContext(r.Context()) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) + return + } + var existingEnv []byte + if existing != nil { + existingEnv = existing.EncryptedEnv + } + envJSON, err := store.MergeSecureCLIEnv(existingEnv, body.Env) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgGrantEnvValueInvalid, err.Error())}) return } - if err := h.store.SetUserCredentials(r.Context(), binaryID, userID, body.Env); err != nil { + if err := h.store.SetUserCredentials(r.Context(), binaryID, userID, envJSON); err != nil { locale := store.LocaleFromContext(r.Context()) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) return diff --git a/internal/store/secure_cli_env.go b/internal/store/secure_cli_env.go new file mode 100644 index 00000000..fc8ded35 --- /dev/null +++ b/internal/store/secure_cli_env.go @@ -0,0 +1,200 @@ +package store + +import ( + "bytes" + "encoding/json" + "fmt" + "sort" + "strings" +) + +const ( + SecureCLIEnvKindSensitive = "sensitive" + SecureCLIEnvKindValue = "value" +) + +// SecureCLIEnvEntry is the stored per-key env representation; legacy KEY:string maps decode as sensitive. +type SecureCLIEnvEntry struct { + Kind string `json:"kind"` + Value string `json:"value"` +} + +// SecureCLIEnvResponseEntry is safe to serialize in admin API responses. +type SecureCLIEnvResponseEntry struct { + Kind string `json:"kind"` + Value *string `json:"value"` + Masked bool `json:"masked"` +} + +func ParseSecureCLIEnv(raw []byte) (map[string]SecureCLIEnvEntry, error) { + if len(bytes.TrimSpace(raw)) == 0 { + return map[string]SecureCLIEnvEntry{}, nil + } + + var payload map[string]json.RawMessage + if err := json.Unmarshal(raw, &payload); err != nil { + return nil, err + } + + env := make(map[string]SecureCLIEnvEntry, len(payload)) + for key, item := range payload { + key = strings.TrimSpace(key) + if key == "" { + continue + } + entry, err := parseSecureCLIEnvEntry(item) + if err != nil { + return nil, fmt.Errorf("%s: %w", key, err) + } + env[key] = entry + } + return env, nil +} + +func parseSecureCLIEnvEntry(raw json.RawMessage) (SecureCLIEnvEntry, error) { + var legacy string + if err := json.Unmarshal(raw, &legacy); err == nil { + return SecureCLIEnvEntry{Kind: SecureCLIEnvKindSensitive, Value: legacy}, nil + } + trimmed := bytes.TrimSpace(raw) + if len(trimmed) > 0 && trimmed[0] != '{' { + value, err := secureCLIEnvValueAsString(raw) + if err != nil { + return SecureCLIEnvEntry{}, err + } + return SecureCLIEnvEntry{Kind: SecureCLIEnvKindSensitive, Value: value}, nil + } + + var obj struct { + Kind string `json:"kind"` + Value json.RawMessage `json:"value"` + } + if err := json.Unmarshal(raw, &obj); err != nil { + return SecureCLIEnvEntry{}, err + } + + kind := strings.ToLower(strings.TrimSpace(obj.Kind)) + if kind == "" { + kind = SecureCLIEnvKindSensitive + } + if kind != SecureCLIEnvKindSensitive && kind != SecureCLIEnvKindValue { + return SecureCLIEnvEntry{}, fmt.Errorf("invalid env kind %q", obj.Kind) + } + + value, err := secureCLIEnvValueAsString(obj.Value) + if err != nil { + return SecureCLIEnvEntry{}, err + } + return SecureCLIEnvEntry{Kind: kind, Value: value}, nil +} + +func secureCLIEnvValueAsString(raw json.RawMessage) (string, error) { + if len(bytes.TrimSpace(raw)) == 0 || bytes.Equal(bytes.TrimSpace(raw), []byte("null")) { + return "", nil + } + var s string + if err := json.Unmarshal(raw, &s); err == nil { + return s, nil + } + var b bool + if err := json.Unmarshal(raw, &b); err == nil { + if b { + return "true", nil + } + return "false", nil + } + var f float64 + if err := json.Unmarshal(raw, &f); err == nil { + return fmt.Sprint(f), nil + } + return "", fmt.Errorf("env value must be string, number, bool, or null") +} + +func SerializeSecureCLIEnv(env map[string]SecureCLIEnvEntry) ([]byte, error) { + normalized := make(map[string]SecureCLIEnvEntry, len(env)) + for key, entry := range env { + key = strings.TrimSpace(key) + if key == "" { + continue + } + kind := strings.ToLower(strings.TrimSpace(entry.Kind)) + if kind == "" { + kind = SecureCLIEnvKindSensitive + } + if kind != SecureCLIEnvKindSensitive && kind != SecureCLIEnvKindValue { + return nil, fmt.Errorf("%s: invalid env kind %q", key, entry.Kind) + } + normalized[key] = SecureCLIEnvEntry{Kind: kind, Value: entry.Value} + } + return json.Marshal(normalized) +} + +func FlattenSecureCLIEnv(raw []byte) (map[string]string, error) { + entries, err := ParseSecureCLIEnv(raw) + if err != nil { + return nil, err + } + flat := make(map[string]string, len(entries)) + for key, entry := range entries { + flat[key] = entry.Value + } + return flat, nil +} + +func MergeSecureCLIEnv(existingJSON []byte, incoming json.RawMessage) ([]byte, error) { + existing, err := ParseSecureCLIEnv(existingJSON) + if err != nil { + return nil, fmt.Errorf("parse existing env: %w", err) + } + incomingEntries, err := ParseSecureCLIEnv(incoming) + if err != nil { + return nil, fmt.Errorf("parse incoming env: %w", err) + } + + out := make(map[string]SecureCLIEnvEntry, len(incomingEntries)) + for key, entry := range incomingEntries { + if entry.Kind == SecureCLIEnvKindSensitive && entry.Value == "" { + if prev, ok := existing[key]; ok { + prev.Kind = SecureCLIEnvKindSensitive + out[key] = prev + continue + } + } + out[key] = entry + } + return SerializeSecureCLIEnv(out) +} + +func SecureCLIEnvKeys(raw []byte) []string { + env, err := ParseSecureCLIEnv(raw) + if err != nil { + return []string{} + } + keys := make([]string, 0, len(env)) + for key := range env { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} + +func SanitizeSecureCLIEnv(env map[string]SecureCLIEnvEntry) map[string]SecureCLIEnvResponseEntry { + out := make(map[string]SecureCLIEnvResponseEntry, len(env)) + for key, entry := range env { + if entry.Kind == SecureCLIEnvKindValue { + value := entry.Value + out[key] = SecureCLIEnvResponseEntry{Kind: SecureCLIEnvKindValue, Value: &value, Masked: false} + continue + } + out[key] = SecureCLIEnvResponseEntry{Kind: SecureCLIEnvKindSensitive, Value: nil, Masked: true} + } + return out +} + +func SanitizeSecureCLIEnvJSON(raw []byte) map[string]SecureCLIEnvResponseEntry { + env, err := ParseSecureCLIEnv(raw) + if err != nil { + return map[string]SecureCLIEnvResponseEntry{} + } + return SanitizeSecureCLIEnv(env) +} diff --git a/internal/store/secure_cli_env_test.go b/internal/store/secure_cli_env_test.go new file mode 100644 index 00000000..2a678b84 --- /dev/null +++ b/internal/store/secure_cli_env_test.go @@ -0,0 +1,102 @@ +package store + +import ( + "encoding/json" + "testing" +) + +func TestParseSecureCLIEnvLegacyMapDefaultsSensitive(t *testing.T) { + env, err := ParseSecureCLIEnv([]byte(`{"TOKEN":"secret","PUBLIC_BASE_URL":"https://goclaw.sh"}`)) + if err != nil { + t.Fatalf("ParseSecureCLIEnv() error = %v", err) + } + if got := env["TOKEN"].Kind; got != SecureCLIEnvKindSensitive { + t.Fatalf("TOKEN kind = %q, want %q", got, SecureCLIEnvKindSensitive) + } + if got := env["TOKEN"].Value; got != "secret" { + t.Fatalf("TOKEN value = %q", got) + } + if got := env["PUBLIC_BASE_URL"].Kind; got != SecureCLIEnvKindSensitive { + t.Fatalf("PUBLIC_BASE_URL kind = %q, want default sensitive", got) + } +} + +func TestParseSecureCLIEnvLegacyScalarsDefaultSensitive(t *testing.T) { + env, err := ParseSecureCLIEnv([]byte(`{"MAX_UPLOAD_SIZE_MB":100,"DEBUG":true}`)) + if err != nil { + t.Fatalf("ParseSecureCLIEnv() error = %v", err) + } + if got := env["MAX_UPLOAD_SIZE_MB"]; got.Kind != SecureCLIEnvKindSensitive || got.Value != "100" { + t.Fatalf("MAX_UPLOAD_SIZE_MB = %#v, want sensitive 100", got) + } + if got := env["DEBUG"]; got.Kind != SecureCLIEnvKindSensitive || got.Value != "true" { + t.Fatalf("DEBUG = %#v, want sensitive true", got) + } +} + +func TestSanitizeSecureCLIEnvMasksSensitiveAndReturnsValues(t *testing.T) { + env := map[string]SecureCLIEnvEntry{ + "TOKEN": {Kind: SecureCLIEnvKindSensitive, Value: "secret"}, + "PUBLIC_BASE_URL": {Kind: SecureCLIEnvKindValue, Value: "https://goclaw.sh"}, + } + got := SanitizeSecureCLIEnv(env) + + if got["TOKEN"].Value != nil { + t.Fatalf("sensitive value leaked: %q", *got["TOKEN"].Value) + } + if !got["TOKEN"].Masked { + t.Fatalf("sensitive masked = false") + } + if got["PUBLIC_BASE_URL"].Value == nil || *got["PUBLIC_BASE_URL"].Value != "https://goclaw.sh" { + t.Fatalf("value entry not returned: %#v", got["PUBLIC_BASE_URL"]) + } + if got["PUBLIC_BASE_URL"].Masked { + t.Fatalf("value entry masked = true") + } +} + +func TestMergeSecureCLIEnvPreservesExistingSensitiveOnEmptyValue(t *testing.T) { + existing := []byte(`{"TOKEN":{"kind":"sensitive","value":"old"},"PUBLIC_BASE_URL":{"kind":"value","value":"https://old.example"}}`) + incoming := json.RawMessage(`{"TOKEN":{"kind":"sensitive","value":""},"PUBLIC_BASE_URL":{"kind":"value","value":"https://new.example"}}`) + + merged, err := MergeSecureCLIEnv(existing, incoming) + if err != nil { + t.Fatalf("MergeSecureCLIEnv() error = %v", err) + } + env, err := ParseSecureCLIEnv(merged) + if err != nil { + t.Fatalf("ParseSecureCLIEnv(merged) error = %v", err) + } + if got := env["TOKEN"].Value; got != "old" { + t.Fatalf("TOKEN value = %q, want preserved old", got) + } + if got := env["PUBLIC_BASE_URL"].Value; got != "https://new.example" { + t.Fatalf("PUBLIC_BASE_URL = %q", got) + } + if got := env["PUBLIC_BASE_URL"].Kind; got != SecureCLIEnvKindValue { + t.Fatalf("PUBLIC_BASE_URL kind = %q", got) + } +} + +func TestFlattenSecureCLIEnvSupportsEntryShape(t *testing.T) { + got, err := FlattenSecureCLIEnv([]byte(`{ + "TOKEN":{"kind":"sensitive","value":"secret"}, + "PUBLIC_BASE_URL":{"kind":"value","value":"https://goclaw.sh"} + }`)) + if err != nil { + t.Fatalf("FlattenSecureCLIEnv() error = %v", err) + } + if got["TOKEN"] != "secret" { + t.Fatalf("TOKEN = %q", got["TOKEN"]) + } + if got["PUBLIC_BASE_URL"] != "https://goclaw.sh" { + t.Fatalf("PUBLIC_BASE_URL = %q", got["PUBLIC_BASE_URL"]) + } +} + +func TestParseSecureCLIEnvRejectsInvalidKind(t *testing.T) { + _, err := ParseSecureCLIEnv([]byte(`{"TOKEN":{"kind":"plain","value":"secret"}}`)) + if err == nil { + t.Fatalf("ParseSecureCLIEnv() error = nil, want invalid kind error") + } +} diff --git a/internal/store/secure_cli_store.go b/internal/store/secure_cli_store.go index 2c8f117c..bac2a08d 100644 --- a/internal/store/secure_cli_store.go +++ b/internal/store/secure_cli_store.go @@ -37,6 +37,8 @@ type SecureCLIBinary struct { UserEnv []byte `json:"-" db:"-"` // per-user encrypted env (populated by LookupByBinary LEFT JOIN) // EnvKeys is set by HTTP handlers only (names from decrypted env, no values); not a DB column. EnvKeys []string `json:"env_keys,omitempty" db:"-"` + // Env is set by HTTP handlers only. Sensitive values are masked; value entries are visible. + Env map[string]SecureCLIEnvResponseEntry `json:"env,omitempty" db:"-"` // AgentGrantsSummary is populated by List only — lightweight per-grant summary (no env bytes). AgentGrantsSummary []AgentGrantSummary `json:"agent_grants_summary" db:"-"` } @@ -92,6 +94,8 @@ type SecureCLIAgentGrant struct { 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:"-"` + // Env is populated by HTTP handlers only for sanitized responses. + Env map[string]SecureCLIEnvResponseEntry `json:"env,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"` diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index a15b5dbc..44efd721 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -39,8 +39,8 @@ var wrapperBinaries = map[string]bool{ // normalizeBinaryName returns the lowercased file base of a binary reference. // Examples: "/usr/bin/gh" → "gh", "./GH" → "gh", " Gh " → "gh". -// Applied at BOTH the gate lookup and lookupCredentialedBinary so the two -// layers agree on identity. (Red Team F5) +// Applied at both the gate lookup and lookupCredentialedBinary so the two +// layers agree on identity. func normalizeBinaryName(s string) string { return filepath.Base(strings.TrimSpace(strings.ToLower(s))) } @@ -436,13 +436,15 @@ func mergeCredentialedEnv(cred *store.SecureCLIBinary) (map[string]string, error return envMap, nil } if len(cred.EncryptedEnv) > 0 { - if err := json.Unmarshal(cred.EncryptedEnv, &envMap); err != nil { + baseEnv, err := store.FlattenSecureCLIEnv(cred.EncryptedEnv) + if err != nil { return nil, err } + maps.Copy(envMap, baseEnv) } if len(cred.UserEnv) > 0 { - var userEnvMap map[string]string - if err := json.Unmarshal(cred.UserEnv, &userEnvMap); err != nil { + userEnvMap, err := store.FlattenSecureCLIEnv(cred.UserEnv) + if err != nil { return nil, err } maps.Copy(envMap, userEnvMap) @@ -628,7 +630,7 @@ func (t *ExecTool) lookupCredentialedBinary(ctx context.Context, command string) } // Normalize lookup key so path/case variants (/usr/bin/gh, ./gh, GH) all // resolve to the same registry row. Same helper is used by the gate - // branch in Execute — identity must agree at both layers. (Red Team F5) + // branch in Execute because identity must agree at both layers. normBinary := normalizeBinaryName(binary) // Get agent ID from context for scoped lookup agentID := store.AgentIDFromContext(ctx) diff --git a/internal/tools/credentialed_exec_env_test.go b/internal/tools/credentialed_exec_env_test.go index e164a8ae..91e48cb5 100644 --- a/internal/tools/credentialed_exec_env_test.go +++ b/internal/tools/credentialed_exec_env_test.go @@ -43,3 +43,24 @@ func TestMergeCredentialedEnvFailsClosedOnInvalidUserEnv(t *testing.T) { t.Fatal("expected invalid per-user env JSON to fail closed") } } + +func TestMergeCredentialedEnvFlattensSensitiveValueEntries(t *testing.T) { + binary := &store.SecureCLIBinary{ + EncryptedEnv: []byte(`{ + "TOKEN":{"kind":"sensitive","value":"secret"}, + "PUBLIC_BASE_URL":{"kind":"value","value":"https://goclaw.sh"} + }`), + UserEnv: []byte(`{"PUBLIC_BASE_URL":{"kind":"value","value":"https://user.example"}}`), + } + + env, err := mergeCredentialedEnv(binary) + if err != nil { + t.Fatalf("mergeCredentialedEnv() error = %v", err) + } + if env["TOKEN"] != "secret" { + t.Fatalf("TOKEN = %q", env["TOKEN"]) + } + if env["PUBLIC_BASE_URL"] != "https://user.example" { + t.Fatalf("PUBLIC_BASE_URL = %q", env["PUBLIC_BASE_URL"]) + } +} diff --git a/ui/web/src/i18n/locales/en/cli-credentials.json b/ui/web/src/i18n/locales/en/cli-credentials.json index a668eb86..c2a15e1e 100644 --- a/ui/web/src/i18n/locales/en/cli-credentials.json +++ b/ui/web/src/i18n/locales/en/cli-credentials.json @@ -39,6 +39,10 @@ "noEnvVarsHint": "Click \"Add Variable\" to define environment variables for this CLI tool.", "envKeyPlaceholder": "ENV_VAR_NAME", "envValuePlaceholder": "value", + "envKindSensitive": "Sensitive", + "envKindValue": "Value", + "envValueWarning": "Plaintext values are visible to authorized users.", + "envValueSuspicious": "This plaintext value looks like a token, password, or key.", "invalidEnvKey": "\"{{key}}\" is not a valid env variable name (use A-Z, 0-9, underscore).", "binaryPathHint": "Leave blank to auto-detect from PATH", "checkBinary": "Check", diff --git a/ui/web/src/i18n/locales/vi/cli-credentials.json b/ui/web/src/i18n/locales/vi/cli-credentials.json index 9cd7f738..9d2a5889 100644 --- a/ui/web/src/i18n/locales/vi/cli-credentials.json +++ b/ui/web/src/i18n/locales/vi/cli-credentials.json @@ -39,6 +39,10 @@ "noEnvVarsHint": "Nhấn \"Thêm biến\" để khai báo biến môi trường cho công cụ CLI này.", "envKeyPlaceholder": "TÊN_BIẾN", "envValuePlaceholder": "giá trị", + "envKindSensitive": "Nhạy cảm", + "envKindValue": "Giá trị", + "envValueWarning": "Giá trị plaintext hiển thị với người dùng có quyền.", + "envValueSuspicious": "Giá trị plaintext này giống token, mật khẩu hoặc key.", "invalidEnvKey": "\"{{key}}\" không phải tên biến môi trường hợp lệ (dùng A-Z, 0-9, gạch dưới).", "binaryPathHint": "Để trống để tự động tìm từ PATH", "checkBinary": "Kiểm tra", diff --git a/ui/web/src/i18n/locales/zh/cli-credentials.json b/ui/web/src/i18n/locales/zh/cli-credentials.json index b0e4d929..ceca7f4e 100644 --- a/ui/web/src/i18n/locales/zh/cli-credentials.json +++ b/ui/web/src/i18n/locales/zh/cli-credentials.json @@ -39,6 +39,10 @@ "noEnvVarsHint": "点击\"添加变量\"为此 CLI 工具定义环境变量。", "envKeyPlaceholder": "变量名", "envValuePlaceholder": "值", + "envKindSensitive": "敏感", + "envKindValue": "值", + "envValueWarning": "明文值对有权限的用户可见。", + "envValueSuspicious": "此明文值看起来像令牌、密码或密钥。", "invalidEnvKey": "\"{{key}}\" 不是有效的环境变量名(使用 A-Z、0-9、下划线)。", "binaryPathHint": "留空自动从 PATH 检测", "checkBinary": "检查", 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 index 7f290f11..ce54fe45 100644 --- 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 @@ -9,7 +9,7 @@ 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 }] }, + { overrideEnabled: true, entries: [{ key: "TOKEN", value: "", kind: "sensitive", masked: true }] }, true, ); expect(payload).toBeUndefined(); @@ -20,14 +20,14 @@ describe("cli credential grant env helpers", () => { { overrideEnabled: true, entries: [ - { key: " CLI_ENV ", value: "agent-value", masked: false }, - { key: "", value: "ignored", masked: false }, - { key: "MASKED", value: "", masked: true }, + { key: " CLI_ENV ", value: "agent-value", kind: "value", masked: false }, + { key: "", value: "ignored", kind: "sensitive", masked: false }, + { key: "MASKED", value: "", kind: "sensitive", masked: true }, ], }, false, ); - expect(payload).toEqual({ CLI_ENV: "agent-value" }); + expect(payload).toEqual({ CLI_ENV: { kind: "value", value: "agent-value" } }); }); it("clears existing env override when override is disabled", () => { @@ -39,13 +39,31 @@ describe("cli credential grant env helpers", () => { const state = envStateFromGrant({ env_set: true, env_keys: ["API_KEY", "TOKEN"], - } as CLIAgentGrant); + } as unknown as CLIAgentGrant); expect(state).toEqual({ overrideEnabled: true, entries: [ - { key: "API_KEY", value: "", masked: true }, - { key: "TOKEN", value: "", masked: true }, + { key: "API_KEY", value: "", kind: "sensitive", masked: true }, + { key: "TOKEN", value: "", kind: "sensitive", masked: true }, + ], + }); + }); + + it("derives visible value entries from sanitized grant env metadata", () => { + const state = envStateFromGrant({ + env_set: true, + env: { + PUBLIC_BASE_URL: { kind: "value", value: "https://goclaw.sh", masked: false }, + TOKEN: { kind: "sensitive", value: null, masked: true }, + }, + } as unknown as CLIAgentGrant); + + expect(state).toEqual({ + overrideEnabled: true, + entries: [ + { key: "PUBLIC_BASE_URL", value: "https://goclaw.sh", kind: "value", masked: false }, + { key: "TOKEN", value: "", kind: "sensitive", masked: true }, ], }); }); diff --git a/ui/web/src/pages/cli-credentials/cli-credential-env-vars-section.tsx b/ui/web/src/pages/cli-credentials/cli-credential-env-vars-section.tsx index 89fa83ab..528bf6fe 100644 --- a/ui/web/src/pages/cli-credentials/cli-credential-env-vars-section.tsx +++ b/ui/web/src/pages/cli-credentials/cli-credential-env-vars-section.tsx @@ -4,11 +4,14 @@ import { Plus, X } from "lucide-react"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; +import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group"; import type { CLIPreset } from "./hooks/use-cli-credentials"; +import type { CLIEnvEntryKind } from "@/types/cli-credential"; export interface ManualEnvEntry { key: string; value: string; + kind: CLIEnvEntryKind; } interface CliCredentialEnvVarsSectionProps { @@ -20,6 +23,10 @@ interface CliCredentialEnvVarsSectionProps { setManualEnvEntries: (updater: (prev: ManualEnvEntry[]) => ManualEnvEntry[]) => void; } +const SUSPICIOUS_VALUE_RE = /(api[_-]?key|token|secret|password|credential|bearer\s+[a-z0-9._-]+|sk-[a-z0-9_-]{12,}|gh[pousr]_[a-z0-9_]{20,})/i; +export const isSuspiciousPlaintextEnv = (key: string, value: string) => + SUSPICIOUS_VALUE_RE.test(`${key}=${value}`); + /** Env var inputs: preset-driven fields or free-form key/value pairs in manual mode. */ export function CliCredentialEnvVarsSection({ isManualMode, @@ -33,14 +40,14 @@ export function CliCredentialEnvVarsSection({ const { t: tc } = useTranslation("common"); const addEntry = useCallback(() => { - setManualEnvEntries((prev) => [...prev, { key: "", value: "" }]); + setManualEnvEntries((prev) => [...prev, { key: "", value: "", kind: "sensitive" }]); }, [setManualEnvEntries]); const removeEntry = useCallback((index: number) => { setManualEnvEntries((prev) => prev.filter((_, i) => i !== index)); }, [setManualEnvEntries]); - const updateEntry = useCallback((index: number, field: "key" | "value", val: string) => { + const updateEntry = useCallback((index: number, field: "key" | "value" | "kind", val: string) => { setManualEnvEntries((prev) => prev.map((entry, i) => (i === index ? { ...entry, [field]: val } : entry)), ); @@ -88,34 +95,57 @@ export function CliCredentialEnvVarsSection({

{t("form.noEnvVarsHint")}

)} {manualEnvEntries.map((entry, idx) => ( -
-
- updateEntry(idx, "key", e.target.value)} - className="text-base md:text-sm font-mono" - /> +
+
+
+ updateEntry(idx, "key", e.target.value)} + className="text-base md:text-sm font-mono" + /> +
+
+ updateEntry(idx, "value", e.target.value)} + className="text-base md:text-sm" + /> +
+
-
- updateEntry(idx, "value", e.target.value)} - className="text-base md:text-sm" - /> -
- + + + + {entry.kind === "value" && ( +

+ {isSuspiciousPlaintextEnv(entry.key, entry.value) + ? t("form.envValueSuspicious") + : t("form.envValueWarning")} +

+ )}
))}
diff --git a/ui/web/src/pages/cli-credentials/cli-credential-form-dialog.tsx b/ui/web/src/pages/cli-credentials/cli-credential-form-dialog.tsx index e7cfc08e..edeb906d 100644 --- a/ui/web/src/pages/cli-credentials/cli-credential-form-dialog.tsx +++ b/ui/web/src/pages/cli-credentials/cli-credential-form-dialog.tsx @@ -17,6 +17,7 @@ import { CliCredentialEnvVarsSection } from "./cli-credential-env-vars-section"; import { CliCredentialBinaryFields } from "./cli-credential-binary-fields"; import { CliCredentialScopeFields } from "./cli-credential-scope-fields"; import { cliCredentialSchema, type CliCredentialFormData } from "@/schemas/credential.schema"; +import type { CLIEnvEntryResponse, CLIEnvPayload } from "@/types/cli-credential"; interface Props { open: boolean; @@ -29,6 +30,20 @@ interface Props { const NONE_PRESET = "__none__"; const ENV_KEY_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/; +function manualEntriesFromEnv( + env: Record | undefined, + fallbackKeys: string[], +): ManualEnvEntry[] { + if (env && Object.keys(env).length > 0) { + return Object.entries(env).map(([key, entry]) => ({ + key, + value: entry.value ?? "", + kind: entry.kind ?? "sensitive", + })); + } + return fallbackKeys.map((key) => ({ key, value: "", kind: "sensitive" })); +} + export function CliCredentialFormDialog({ open, onOpenChange, credential, presets, onSubmit }: Props) { const { t } = useTranslation("cli-credentials"); const { t: tc } = useTranslation("common"); @@ -91,13 +106,13 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset return; } - const applyEnvKeys = (keys: string[]) => { + const applyEnvState = (env: Record | undefined, keys: string[]) => { setInitialEnvKeys(keys); - setManualEnvEntries(keys.length > 0 ? keys.map((k) => ({ key: k, value: "" })) : []); + setManualEnvEntries(manualEntriesFromEnv(env, keys)); }; if (credential.env_keys !== undefined) { - applyEnvKeys(credential.env_keys ?? []); + applyEnvState(credential.env, credential.env_keys ?? []); return; } @@ -106,9 +121,9 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset try { const full = await http.get(`/v1/cli-credentials/${credential.id}`); if (cancelled) return; - applyEnvKeys(full.env_keys ?? []); + applyEnvState(full.env, full.env_keys ?? []); } catch { - if (!cancelled) applyEnvKeys([]); + if (!cancelled) applyEnvState(undefined, []); } })(); return () => { cancelled = true; }; @@ -156,16 +171,22 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset const splitCommaList = (v: string): string[] => v.split(",").map((s) => s.trim()).filter(Boolean); - const buildEnvPayload = (): Record | null => { - if (!isManualMode) return envValues; - const env: Record = {}; + const buildEnvPayload = (): CLIEnvPayload | null => { + if (!isManualMode) { + const presetEnv: CLIEnvPayload = {}; + for (const [key, value] of Object.entries(envValues)) { + presetEnv[key] = { kind: "sensitive", value }; + } + return presetEnv; + } + const env: CLIEnvPayload = {}; for (const entry of manualEnvEntries) { const k = entry.key.trim(); if (k && !ENV_KEY_PATTERN.test(k)) { setError(t("form.invalidEnvKey", { key: k })); return null; } - if (k) env[k] = entry.value; + if (k) env[k] = { kind: entry.kind, value: entry.value }; } return env; }; diff --git a/ui/web/src/pages/cli-credentials/cli-credential-grant-env-row.tsx b/ui/web/src/pages/cli-credentials/cli-credential-grant-env-row.tsx new file mode 100644 index 00000000..1d14eada --- /dev/null +++ b/ui/web/src/pages/cli-credentials/cli-credential-grant-env-row.tsx @@ -0,0 +1,73 @@ +import { X } from "lucide-react"; +import { useTranslation } from "react-i18next"; +import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; +import { isSuspiciousPlaintextEnv } from "./cli-credential-env-vars-section"; +import type { GrantEnvEntry } from "./cli-credential-grant-env-section"; + +interface Props { + entry: GrantEnvEntry; + hasError: boolean; + onRemove: () => void; + onUpdate: (field: "key" | "value" | "kind", value: string) => void; +} + +export function CliCredentialGrantEnvRow({ entry, hasError, onRemove, onUpdate }: Props) { + const { t } = useTranslation("cli-credentials"); + + return ( +
+
+ onUpdate("key", e.target.value)} + className={`text-base md:text-sm font-mono${hasError ? " border-destructive" : ""}`} + /> + {hasError && ( +

+ {t("grants.envVars.deniedKey", { key: entry.key })} +

+ )} +
+
+ {entry.masked ? ( + + ) : ( + onUpdate("value", e.target.value)} + className="text-base md:text-sm" + /> + )} +
+ {!entry.masked && ( + + )} + + {!entry.masked && entry.kind === "value" && ( +

+ {isSuspiciousPlaintextEnv(entry.key, entry.value) + ? t("form.envValueSuspicious") + : t("form.envValueWarning")} +

+ )} +
+ ); +} diff --git a/ui/web/src/pages/cli-credentials/cli-credential-grant-env-section.tsx b/ui/web/src/pages/cli-credentials/cli-credential-grant-env-section.tsx index ebff9a70..5e546540 100644 --- a/ui/web/src/pages/cli-credentials/cli-credential-grant-env-section.tsx +++ b/ui/web/src/pages/cli-credentials/cli-credential-grant-env-section.tsx @@ -6,13 +6,14 @@ */ import { useState, useCallback, useEffect, useRef } from "react"; import { useTranslation } from "react-i18next"; -import { Plus, X, Eye } from "lucide-react"; +import { Plus, Eye } from "lucide-react"; import { Button } from "@/components/ui/button"; -import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; import { Switch } from "@/components/ui/switch"; import { toast } from "@/stores/use-toast-store"; import { useHttp } from "@/hooks/use-ws"; +import type { CLIEnvEntryKind } from "@/types/cli-credential"; +import { CliCredentialGrantEnvRow } from "./cli-credential-grant-env-row"; // Keep in sync with internal/crypto/env_denylist.go. // Backend is authoritative; this list drives inline UX warnings only. @@ -23,7 +24,7 @@ const ENV_DENYLIST_EXACT = new Set([ "PYTHONPATH", "PYTHONHOME", "PYTHONSTARTUP", "GIT_SSH_COMMAND", "GIT_SSH", "GIT_EXEC_PATH", "GIT_CONFIG_SYSTEM", "SSH_AUTH_SOCK", - // Finding #6 additions — keep in sync with internal/crypto/env_denylist.go + // Shell startup and proxy/certificate variables can alter command behavior. "BASH_ENV", "ENV", "PROMPT_COMMAND", "PERL5LIB", "RUBYOPT", "HTTPS_PROXY", "HTTP_PROXY", "NO_PROXY", @@ -36,6 +37,7 @@ const ENV_DENYLIST_PREFIXES = ["DYLD_", "GOCLAW_", "LD_", "NPM_CONFIG_"]; export interface GrantEnvEntry { key: string; value: string; + kind: CLIEnvEntryKind; masked: boolean; // true = not yet revealed from server } @@ -63,10 +65,10 @@ export function CliCredentialGrantEnvSection({ const [revealing, setRevealing] = useState(false); const [revealed, setRevealed] = useState(false); const { overrideEnabled, entries } = state; - // Finding #10: track blur timeout so we can cancel it on reveal/unmount. + // Track reveal timeout so plaintext can be cleared on reveal refresh/unmount. const blurTimeoutRef = useRef | null>(null); - // Finding #10: clear revealed plaintext from entries on component unmount. + // Clear revealed plaintext from entries on component unmount. // This is defense-in-depth — plaintext should not persist in React state beyond use. useEffect(() => { return () => { @@ -77,7 +79,6 @@ export function CliCredentialGrantEnvSection({ entries: state.entries.map((e) => ({ ...e, value: "", masked: e.masked })), }); }; - // eslint-disable-next-line react-hooks/exhaustive-deps }, []); const setEntries = useCallback( @@ -89,10 +90,10 @@ export function CliCredentialGrantEnvSection({ const handleToggle = useCallback((checked: boolean) => { if (checked) { if (initialEnvSet && !revealed && entries.every((e) => e.masked)) { - const masked: GrantEnvEntry[] = initialEnvKeys.map((k) => ({ key: k, value: "", masked: true })); - onChange({ overrideEnabled: true, entries: masked.length > 0 ? masked : [{ key: "", value: "", masked: false }] }); + const masked: GrantEnvEntry[] = initialEnvKeys.map((k) => ({ key: k, value: "", kind: "sensitive", masked: true })); + onChange({ overrideEnabled: true, entries: masked.length > 0 ? masked : [{ key: "", value: "", kind: "sensitive", masked: false }] }); } else if (entries.length === 0) { - onChange({ overrideEnabled: true, entries: [{ key: "", value: "", masked: false }] }); + onChange({ overrideEnabled: true, entries: [{ key: "", value: "", kind: "sensitive", masked: false }] }); } else { onChange({ overrideEnabled: true, entries }); } @@ -105,16 +106,19 @@ export function CliCredentialGrantEnvSection({ if (!grantId) return; setRevealing(true); try { - // POST — not GET (C1 red-team). Direct call, not cached by TanStack Query. + // POST keeps reveal out of URL/history and avoids query caching. const res = await http.post<{ env_vars: Record }>( `/v1/cli-credentials/${binaryId}/agent-grants/${grantId}/env:reveal`, ); const filled: GrantEnvEntry[] = Object.entries(res.env_vars).map(([k, v]) => ({ - key: k, value: v, masked: false, + key: k, + value: v, + kind: entries.find((entry) => entry.key === k)?.kind ?? "sensitive", + masked: false, })); onChange({ overrideEnabled: true, entries: filled.length > 0 ? filled : entries }); setRevealed(true); - // Finding #10: wipe plaintext after 30s of inactivity (defense-in-depth). + // Wipe plaintext after 30s of inactivity. if (blurTimeoutRef.current) clearTimeout(blurTimeoutRef.current); blurTimeoutRef.current = setTimeout(() => { onChange({ @@ -133,9 +137,9 @@ export function CliCredentialGrantEnvSection({ } }, [grantId, binaryId, http, onChange, entries, t]); - const addEntry = useCallback(() => setEntries((p) => [...p, { key: "", value: "", masked: false }]), [setEntries]); + const addEntry = useCallback(() => setEntries((p) => [...p, { key: "", value: "", kind: "sensitive", masked: false }]), [setEntries]); const removeEntry = useCallback((i: number) => setEntries((p) => p.filter((_, j) => j !== i)), [setEntries]); - const updateEntry = useCallback((i: number, f: "key" | "value", v: string) => + const updateEntry = useCallback((i: number, f: "key" | "value" | "kind", v: string) => setEntries((p) => p.map((e, j) => j === i ? { ...e, [f]: v, masked: false } : e)), [setEntries]); const isDenied = (k: string) => { @@ -174,32 +178,13 @@ export function CliCredentialGrantEnvSection({ {entries.map((entry, idx) => { const hasError = isDenied(entry.key) || isRejected(entry.key); return ( -
-
- updateEntry(idx, "key", e.target.value)} - className={`text-base md:text-sm font-mono${hasError ? " border-destructive" : ""}`} /> - {hasError && ( -

- {t("grants.envVars.deniedKey", { key: entry.key })} -

- )} -
-
- {entry.masked ? ( - - ) : ( - updateEntry(idx, "value", e.target.value)} - className="text-base md:text-sm" /> - )} -
- -
+ removeEntry(idx)} + onUpdate={(field, value) => updateEntry(idx, field, value)} + /> ); })} {entries.length === 0 && ( diff --git a/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog-helpers.ts b/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog-helpers.ts index 3d5e0ac2..c59d33ea 100644 --- a/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog-helpers.ts +++ b/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog-helpers.ts @@ -3,6 +3,7 @@ */ import type { CLIAgentGrant } from "./hooks/use-cli-credentials"; import type { GrantEnvState, GrantEnvEntry } from "./cli-credential-grant-env-section"; +import type { CLIEnvPayload } from "@/types/cli-credential"; export const EMPTY_ENV_STATE: GrantEnvState = { overrideEnabled: false, entries: [] }; @@ -15,14 +16,14 @@ export const EMPTY_ENV_STATE: GrantEnvState = { overrideEnabled: false, entries: export function buildEnvVarsPayload( envState: GrantEnvState, originalEnvSet: boolean, -): Record | null | undefined { +): CLIEnvPayload | null | undefined { const { overrideEnabled, entries } = envState; if (overrideEnabled) { const allMasked = entries.length > 0 && entries.every((e: GrantEnvEntry) => e.masked); if (allMasked) return undefined; // not revealed; don't overwrite - const result: Record = {}; + const result: CLIEnvPayload = {}; for (const e of entries) { - if (!e.masked && e.key.trim()) result[e.key.trim()] = e.value; + if (!e.masked && e.key.trim()) result[e.key.trim()] = { kind: e.kind, value: e.value }; } return result; } @@ -31,10 +32,23 @@ export function buildEnvVarsPayload( /** Derive initial GrantEnvState from an existing grant. */ export function envStateFromGrant(grant: CLIAgentGrant): GrantEnvState { + if (grant.env && Object.keys(grant.env).length > 0) { + return { + overrideEnabled: true, + entries: Object.entries(grant.env) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([key, entry]) => ({ + key, + value: entry.kind === "value" && entry.value !== null ? entry.value : "", + kind: entry.kind, + masked: entry.masked, + })), + }; + } if (grant.env_set && grant.env_keys && grant.env_keys.length > 0) { return { overrideEnabled: true, - entries: grant.env_keys.map((k) => ({ key: k, value: "", masked: true })), + entries: grant.env_keys.map((k) => ({ key: k, value: "", kind: "sensitive", masked: true })), }; } return EMPTY_ENV_STATE; diff --git a/ui/web/src/pages/cli-credentials/cli-user-credentials-dialog.tsx b/ui/web/src/pages/cli-credentials/cli-user-credentials-dialog.tsx index 7cfe6c57..79a95a9a 100644 --- a/ui/web/src/pages/cli-credentials/cli-user-credentials-dialog.tsx +++ b/ui/web/src/pages/cli-credentials/cli-user-credentials-dialog.tsx @@ -13,11 +13,12 @@ import { Button } from "@/components/ui/button"; import { Label } from "@/components/ui/label"; import { Badge } from "@/components/ui/badge"; import { UserPickerCombobox } from "@/components/shared/user-picker-combobox"; -import { KeyValueEditor } from "@/components/shared/key-value-editor"; import { toast } from "@/stores/use-toast-store"; import { useHttp } from "@/hooks/use-ws"; import i18next from "i18next"; +import { CliCredentialEnvVarsSection, type ManualEnvEntry } from "./cli-credential-env-vars-section"; import type { SecureCLIBinary } from "./hooks/use-cli-credentials"; +import type { CLIEnvEntryResponse, CLIEnvPayload } from "@/types/cli-credential"; interface UserCredEntry { id: string; @@ -36,11 +37,26 @@ interface CLIUserCredentialsDialogProps { binary: SecureCLIBinary; } -const SENSITIVE_ENV_RE = /^.*(key|secret|token|password|credential).*$/i; -const isSensitiveEnv = (key: string) => SENSITIVE_ENV_RE.test(key.trim()); - type ViewState = "list" | "form"; +function entriesFromEnv(env: Record | null | undefined): ManualEnvEntry[] { + if (!env || Object.keys(env).length === 0) return []; + return Object.entries(env).map(([key, entry]) => ({ + key, + value: entry.value ?? "", + kind: entry.kind ?? "sensitive", + })); +} + +function envPayloadFromEntries(entries: ManualEnvEntry[]): CLIEnvPayload { + const env: CLIEnvPayload = {}; + for (const entry of entries) { + const key = entry.key.trim(); + if (key) env[key] = { kind: entry.kind, value: entry.value }; + } + return env; +} + export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUserCredentialsDialogProps) { const { t } = useTranslation("cli-credentials"); const http = useHttp(); @@ -54,7 +70,7 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser const [userId, setUserId] = useState(""); // Separate search text from selected value (onChange fires on every keystroke) const [userSearchText, setUserSearchText] = useState(""); - const [env, setEnv] = useState>({}); + const [envEntries, setEnvEntries] = useState([]); const [saving, setSaving] = useState(false); const [deleting, setDeletingId] = useState(null); @@ -80,7 +96,7 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser setEditEntry(null); setUserId(""); setUserSearchText(""); - setEnv({}); + setEnvEntries([]); loadList(); }, [open, loadList]); @@ -88,7 +104,7 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser setEditEntry(null); setUserId(""); setUserSearchText(""); - setEnv({}); + setEnvEntries([]); setView("form"); }; @@ -96,14 +112,14 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser setEditEntry(entry); setUserId(entry.user_id); setUserSearchText(entry.user_id); - setEnv({}); + setEnvEntries([]); setView("form"); // Load existing env for edit try { - const res = await http.get<{ user_id: string; env: Record | null }>( + const res = await http.get<{ user_id: string; env: Record | null }>( `/v1/cli-credentials/${binary.id}/user-credentials/${entry.user_id}`, ); - setEnv(res.env ?? {}); + setEnvEntries(entriesFromEnv(res.env)); } catch { // leave env empty — user can re-enter } @@ -112,6 +128,7 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser const handleSave = async () => { const uid = userId.trim(); if (!uid) return; + const env = envPayloadFromEntries(envEntries); // New entry needs at least one variable; edits may clear all keys (empty object). if (!editEntry && Object.keys(env).length === 0) { toast.error(i18next.t("cli-credentials:userCredentials.envRequired")); @@ -251,13 +268,13 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser
- undefined} + manualEnvEntries={envEntries} + setManualEnvEntries={setEnvEntries} />
diff --git a/ui/web/src/types/cli-credential.ts b/ui/web/src/types/cli-credential.ts index a9b03854..65c0b36a 100644 --- a/ui/web/src/types/cli-credential.ts +++ b/ui/web/src/types/cli-credential.ts @@ -1,3 +1,18 @@ +export type CLIEnvEntryKind = "sensitive" | "value"; + +export interface CLIEnvEntryInput { + kind: CLIEnvEntryKind; + value: string; +} + +export interface CLIEnvEntryResponse { + kind: CLIEnvEntryKind; + value: string | null; + masked: boolean; +} + +export type CLIEnvPayload = Record; + export interface SecureCLIBinary { id: string; binary_name: string; @@ -14,6 +29,8 @@ export interface SecureCLIBinary { updated_at: string; /** Env variable names only (no values); from API for edit form */ env_keys?: string[]; + /** Sanitized env metadata; sensitive values are masked, value entries include value. */ + env?: Record; /** * Agent grants summary for row chips (Phase 4 API field). * Absent on older API versions — capability-probe: skip rendering if undefined. @@ -49,7 +66,7 @@ export interface CLICredentialInput { tips?: string; is_global?: boolean; enabled?: boolean; - env?: Record; + env?: CLIEnvPayload; } /** Per-agent grant with optional setting overrides */ @@ -66,6 +83,8 @@ export interface CLIAgentGrant { env_set?: boolean; /** Env variable names only (no values); populated when env_set=true */ env_keys?: string[]; + /** Sanitized env metadata; sensitive values are masked, value entries include value. */ + env?: Record; created_at: string; updated_at: string; } @@ -78,7 +97,7 @@ export interface CLIAgentGrantInput { tips?: string | null; enabled?: boolean; /** - * env_vars semantics — 3-state, all three distinct behaviors (Finding #15): + * env_vars semantics — 3-state, all three distinct behaviors: * * - **absent / undefined** → keep existing env override (omit from request payload) * - **null** → clear override; grant falls back to binary-level defaults @@ -88,7 +107,7 @@ export interface CLIAgentGrantInput { * Backend: internal/http/secure_cli_agent_grants.go handleUpdate (3-state env_vars branch). * Keys must match ^[A-Z_][A-Z0-9_]*$ and must not be on the denylist. */ - env_vars?: Record | null; + env_vars?: CLIEnvPayload | null; } /** Summary of a single grant shown in the table row chips (Phase 4 API field). */