From 173f1cea53c5bfc420d7408cea07a60210694b28 Mon Sep 17 00:00:00 2001 From: Huy Doan Date: Thu, 2 Apr 2026 11:59:49 +0700 Subject: [PATCH] fix(cli-credentials): show existing env keys on edit and support merge/remove (#629) fix(cli-credentials): show existing env keys on edit and support merge/remove - Expose env_keys on secure CLI list/get and per-user credential list (values stay hidden) - PUT merges env with stored secrets: empty value keeps existing, removed keys dropped - Generic error messages to prevent env key enumeration - Audit logging for user credential set/delete - DRY: reuse envKeysFromDecryptedJSON helper - Client-side env key name validation (en/vi/zh i18n) --- internal/http/secure_cli.go | 92 ++++++++++++++++++- internal/http/secure_cli_user_credentials.go | 7 +- internal/store/secure_cli_store.go | 2 + .../src/i18n/locales/en/cli-credentials.json | 5 +- .../src/i18n/locales/vi/cli-credentials.json | 5 +- .../src/i18n/locales/zh/cli-credentials.json | 5 +- .../cli-credential-form-dialog.tsx | 54 ++++++++++- .../cli-user-credentials-dialog.tsx | 24 +++-- ui/web/src/types/cli-credential.ts | 2 + 9 files changed, 173 insertions(+), 23 deletions(-) diff --git a/internal/http/secure_cli.go b/internal/http/secure_cli.go index d8ede321..48a94358 100644 --- a/internal/http/secure_cli.go +++ b/internal/http/secure_cli.go @@ -2,9 +2,11 @@ package http import ( "encoding/json" + "fmt" "log/slog" "net/http" "regexp" + "sort" "strings" "github.com/google/uuid" @@ -59,6 +61,72 @@ 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 +} + +// 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 (h *SecureCLIHandler) handleList(w http.ResponseWriter, r *http.Request) { locale := store.LocaleFromContext(r.Context()) result, err := h.store.List(r.Context()) @@ -67,8 +135,9 @@ 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 } - // Mask encrypted env — never send raw credentials to UI + // Never send env values; only variable names for editing. for i := range result { + result[i].EnvKeys = envKeysFromDecryptedJSON(result[i].EncryptedEnv) result[i].EncryptedEnv = nil } writeJSON(w, http.StatusOK, map[string]any{"items": result}) @@ -184,7 +253,8 @@ func (h *SecureCLIHandler) handleGet(w http.ResponseWriter, r *http.Request) { return } - b.EncryptedEnv = nil // don't expose credentials + b.EnvKeys = envKeysFromDecryptedJSON(b.EncryptedEnv) + b.EncryptedEnv = nil // don't expose credential values writeJSON(w, http.StatusOK, b) } @@ -214,10 +284,24 @@ func (h *SecureCLIHandler) handleUpdate(w http.ResponseWriter, r *http.Request) } } - // If env is updated, serialize as JSON string for the store encrypt path + // 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 { - envJSON, _ := json.Marshal(envMap) + 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") } diff --git a/internal/http/secure_cli_user_credentials.go b/internal/http/secure_cli_user_credentials.go index 48e56b99..3ef6a6f0 100644 --- a/internal/http/secure_cli_user_credentials.go +++ b/internal/http/secure_cli_user_credentials.go @@ -23,22 +23,25 @@ func (h *SecureCLIHandler) handleListUserCredentials(w http.ResponseWriter, r *h writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, err.Error())}) return } - // Return without encrypted env for listing (only user_id + timestamps) + // 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"` } entries := make([]entry, 0, len(creds)) for _, c := range creds { + envKeys := envKeysFromDecryptedJSON(c.EncryptedEnv) entries = append(entries, entry{ ID: c.ID, BinaryID: c.BinaryID, UserID: c.UserID, HasEnv: len(c.EncryptedEnv) > 0, + EnvKeys: envKeys, CreatedAt: c.CreatedAt, UpdatedAt: c.UpdatedAt, }) @@ -119,6 +122,7 @@ func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *ht return } + emitAudit(h.msgBus, r, "secure_cli.user_credentials.updated", "secure_cli_user_credentials", binaryID.String()+"/"+userID) h.emitCacheInvalidate("") writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } @@ -142,6 +146,7 @@ func (h *SecureCLIHandler) handleDeleteUserCredentials(w http.ResponseWriter, r return } + emitAudit(h.msgBus, r, "secure_cli.user_credentials.deleted", "secure_cli_user_credentials", binaryID.String()+"/"+userID) h.emitCacheInvalidate("") writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } diff --git a/internal/store/secure_cli_store.go b/internal/store/secure_cli_store.go index 80ef45de..b7d86172 100644 --- a/internal/store/secure_cli_store.go +++ b/internal/store/secure_cli_store.go @@ -23,6 +23,8 @@ type SecureCLIBinary struct { Enabled bool `json:"enabled"` CreatedBy string `json:"created_by"` UserEnv []byte `json:"-"` // 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"` } // SecureCLIUserCredential holds per-user encrypted env overrides for a binary. diff --git a/ui/web/src/i18n/locales/en/cli-credentials.json b/ui/web/src/i18n/locales/en/cli-credentials.json index 4496fef2..d6a4d214 100644 --- a/ui/web/src/i18n/locales/en/cli-credentials.json +++ b/ui/web/src/i18n/locales/en/cli-credentials.json @@ -21,7 +21,7 @@ "presetPlaceholder": "Select a preset (optional)", "noPreset": "No preset (manual)", "presetHint": "Selecting a preset auto-fills fields and shows required env vars.", - "encryptedHint": "Credentials are encrypted and cannot be displayed. Leave environment variables empty to keep existing values, or re-enter to update.", + "encryptedHint": "Values are encrypted and never shown. Variable names are listed below; leave a value empty to keep the stored secret, or type a new value to replace it.", "envVars": "Environment Variables", "binaryName": "Binary Name", "binaryPath": "Binary Path", @@ -37,7 +37,8 @@ "addEnvVar": "Add Variable", "noEnvVarsHint": "Click \"Add Variable\" to define environment variables for this CLI tool.", "envKeyPlaceholder": "ENV_VAR_NAME", - "envValuePlaceholder": "value" + "envValuePlaceholder": "value", + "invalidEnvKey": "\"{{key}}\" is not a valid env variable name (use A-Z, 0-9, underscore)." }, "placeholders": { "binaryName": "e.g. gh", diff --git a/ui/web/src/i18n/locales/vi/cli-credentials.json b/ui/web/src/i18n/locales/vi/cli-credentials.json index 49250dcc..58ef1e70 100644 --- a/ui/web/src/i18n/locales/vi/cli-credentials.json +++ b/ui/web/src/i18n/locales/vi/cli-credentials.json @@ -21,7 +21,7 @@ "presetPlaceholder": "Chọn mẫu (tùy chọn)", "noPreset": "Không dùng mẫu (nhập tay)", "presetHint": "Chọn mẫu sẽ tự động điền các trường và hiển thị biến môi trường cần thiết.", - "encryptedHint": "Thông tin xác thực đã được mã hóa và không thể hiển thị. Để trống biến môi trường để giữ giá trị hiện tại, hoặc nhập lại để cập nhật.", + "encryptedHint": "Giá trị được mã hóa và không hiển thị. Tên biến được liệt kê bên dưới; để trống giá trị để giữ bí mật đã lưu, hoặc nhập giá trị mới để thay thế.", "envVars": "Biến môi trường", "binaryName": "Tên Binary", "binaryPath": "Đường dẫn Binary", @@ -37,7 +37,8 @@ "addEnvVar": "Thêm biến", "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ị" + "envValuePlaceholder": "giá trị", + "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)." }, "placeholders": { "binaryName": "vd: gh", diff --git a/ui/web/src/i18n/locales/zh/cli-credentials.json b/ui/web/src/i18n/locales/zh/cli-credentials.json index 5ef5d977..4d33ed00 100644 --- a/ui/web/src/i18n/locales/zh/cli-credentials.json +++ b/ui/web/src/i18n/locales/zh/cli-credentials.json @@ -21,7 +21,7 @@ "presetPlaceholder": "选择预设（可选）", "noPreset": "不使用预设（手动）", "presetHint": "选择预设会自动填充字段并显示所需的环境变量。", - "encryptedHint": "凭证已加密，无法显示。留空环境变量以保留现有值，或重新输入以更新。", + "encryptedHint": "值已加密，不会显示。下方列出变量名；留空值以保留已保存的密钥，或输入新值以替换。", "envVars": "环境变量", "binaryName": "二进制名称", "binaryPath": "二进制路径", @@ -37,7 +37,8 @@ "addEnvVar": "添加变量", "noEnvVarsHint": "点击\"添加变量\"为此 CLI 工具定义环境变量。", "envKeyPlaceholder": "变量名", - "envValuePlaceholder": "值" + "envValuePlaceholder": "值", + "invalidEnvKey": "\"{{key}}\" 不是有效的环境变量名（使用 A-Z、0-9、下划线）。" }, "placeholders": { "binaryName": "例如 gh", 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 c0f59ba5..27ac2821 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 @@ -12,6 +12,7 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue, } from "@/components/ui/select"; import { Plus, X } from "lucide-react"; +import { useHttp } from "@/hooks/use-ws"; import type { SecureCLIBinary, CLICredentialInput, CLIPreset } from "./hooks/use-cli-credentials"; interface ManualEnvEntry { @@ -32,6 +33,7 @@ const NONE_PRESET = "__none__"; export function CliCredentialFormDialog({ open, onOpenChange, credential, presets, onSubmit }: Props) { const { t } = useTranslation("cli-credentials"); const { t: tc } = useTranslation("common"); + const http = useHttp(); const [selectedPreset, setSelectedPreset] = useState(NONE_PRESET); const [binaryName, setBinaryName] = useState(""); @@ -45,6 +47,8 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset const [enabled, setEnabled] = useState(true); const [envValues, setEnvValues] = useState>({}); const [manualEnvEntries, setManualEnvEntries] = useState([]); + /** Snapshot when dialog opens (edit) — detect clearing all env rows */ + const [initialEnvKeys, setInitialEnvKeys] = useState([]); const [loading, setLoading] = useState(false); const [error, setError] = useState(""); @@ -71,9 +75,38 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset setAgentId(credential?.agent_id ?? ""); setEnabled(credential?.enabled ?? true); setEnvValues({}); - setManualEnvEntries([]); setError(""); - }, [open, credential]); + + if (!credential) { + setInitialEnvKeys([]); + setManualEnvEntries([]); + return; + } + + const applyEnvKeys = (keys: string[]) => { + setInitialEnvKeys(keys); + setManualEnvEntries(keys.length > 0 ? keys.map((k) => ({ key: k, value: "" })) : []); + }; + + if (credential.env_keys !== undefined) { + applyEnvKeys(credential.env_keys ?? []); + return; + } + + let cancelled = false; + void (async () => { + try { + const full = await http.get(`/v1/cli-credentials/${credential.id}`); + if (cancelled) return; + applyEnvKeys(full.env_keys ?? []); + } catch { + if (!cancelled) applyEnvKeys([]); + } + })(); + return () => { + cancelled = true; + }; + }, [open, credential, http]); const applyPreset = (key: string) => { setSelectedPreset(key); @@ -107,12 +140,18 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset const splitCommaList = (v: string): string[] => v.split(",").map((s) => s.trim()).filter(Boolean); - const buildEnvPayload = (): Record => { + const ENV_KEY_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/; + + const buildEnvPayload = (): Record | null => { if (!isManualMode) return envValues; const env: Record = {}; for (const entry of manualEnvEntries) { const k = entry.key.trim(); - if (k && entry.value) env[k] = entry.value; + if (k && !ENV_KEY_PATTERN.test(k)) { + setError(t("form.invalidEnvKey", { key: k })); + return null; + } + if (k) env[k] = entry.value; } return env; }; @@ -138,7 +177,12 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset }; if (selectedPreset !== NONE_PRESET) payload.preset = selectedPreset; const env = buildEnvPayload(); - if (Object.keys(env).length > 0) payload.env = env; + if (!env) return; + if (Object.keys(env).length > 0) { + payload.env = env; + } else if (isEdit && isManualMode && initialEnvKeys.length > 0) { + payload.env = {}; + } await onSubmit(payload); onOpenChange(false); } catch (err) { 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 e1915c92..d777b1cf 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 @@ -24,6 +24,8 @@ interface UserCredEntry { binary_id: string; user_id: string; has_env: boolean; + /** Env variable names (no values) for display */ + env_keys?: string[]; created_at: string; updated_at: string; } @@ -103,7 +105,8 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser const handleSave = async () => { const uid = userId.trim(); if (!uid) return; - if (Object.keys(env).length === 0) { + // 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")); return; } @@ -169,12 +172,19 @@ export function CLIUserCredentialsDialog({ open, onOpenChange, binary }: CLIUser key={entry.id} className="flex items-center justify-between rounded-md border px-3 py-2" > -
- {entry.user_id} - {entry.has_env && ( - - env - +
+
+ {entry.user_id} + {entry.has_env && ( + + env + + )} +
+ {entry.env_keys && entry.env_keys.length > 0 && ( +

+ {entry.env_keys.join(", ")} +

)}
diff --git a/ui/web/src/types/cli-credential.ts b/ui/web/src/types/cli-credential.ts index 1dc57eab..46c10344 100644 --- a/ui/web/src/types/cli-credential.ts +++ b/ui/web/src/types/cli-credential.ts @@ -12,6 +12,8 @@ export interface SecureCLIBinary { created_by: string; created_at: string; updated_at: string; + /** Env variable names only (no values); from API for edit form */ + env_keys?: string[]; } export interface CLIPresetEnvVar {