From 10cd911dc73d355bd7b89e37856a7c2f6cef697f Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 2 Apr 2026 14:57:50 +0700 Subject: [PATCH] feat(secure-cli): check-binary endpoint, agent select dropdown, fix ambiguous column Backend: - Add POST /v1/cli-credentials/check-binary to resolve binary via exec.LookPath - Security: safeBinaryNameRe regex prevents filesystem probing - Fix ambiguous column in LookupByBinary LEFT JOIN (secureCLISelectColsAliased) - Add diagnostic logging to lookupCredentialedBinary Frontend: - Replace agent ID text input with Select dropdown (reuse useAgents hook) - Add Check Binary button next to binary name (auto-fills resolved path) - Add binary path hint explaining auto-detection from PATH - Update i18n keys (en/vi/zh) for new UI elements --- internal/http/secure_cli.go | 34 +++++++ internal/store/pg/secure_cli.go | 8 +- internal/tools/credentialed_exec.go | 9 +- .../src/i18n/locales/en/cli-credentials.json | 13 ++- .../src/i18n/locales/vi/cli-credentials.json | 13 ++- .../src/i18n/locales/zh/cli-credentials.json | 13 ++- .../cli-credential-form-dialog.tsx | 90 +++++++++++++++---- 7 files changed, 150 insertions(+), 30 deletions(-) diff --git a/internal/http/secure_cli.go b/internal/http/secure_cli.go index 48a94358..672c1507 100644 --- a/internal/http/secure_cli.go +++ b/internal/http/secure_cli.go @@ -5,6 +5,7 @@ import ( "fmt" "log/slog" "net/http" + "os/exec" "regexp" "sort" "strings" @@ -19,6 +20,10 @@ import ( "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) +// safeBinaryNameRe allows only simple binary names: alphanumeric, hyphens, underscores, dots. +// No path separators or shell metacharacters — prevents filesystem probing via LookPath. +var safeBinaryNameRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,63}$`) + // SecureCLIHandler handles secure CLI binary credential CRUD endpoints. type SecureCLIHandler struct { store store.SecureCLIStore @@ -35,6 +40,7 @@ func (h *SecureCLIHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("GET /v1/cli-credentials", h.auth(h.handleList)) mux.HandleFunc("POST /v1/cli-credentials", h.auth(h.handleCreate)) mux.HandleFunc("GET /v1/cli-credentials/presets", h.auth(h.handlePresets)) + mux.HandleFunc("POST /v1/cli-credentials/check-binary", h.auth(h.handleCheckBinary)) mux.HandleFunc("GET /v1/cli-credentials/{id}", h.auth(h.handleGet)) mux.HandleFunc("PUT /v1/cli-credentials/{id}", h.auth(h.handleUpdate)) mux.HandleFunc("DELETE /v1/cli-credentials/{id}", h.auth(h.handleDelete)) @@ -343,6 +349,34 @@ func (h *SecureCLIHandler) handlePresets(w http.ResponseWriter, _ *http.Request) writeJSON(w, http.StatusOK, map[string]any{"presets": tools.CLIPresets}) } +// handleCheckBinary resolves a binary name to its absolute path via exec.LookPath. +func (h *SecureCLIHandler) handleCheckBinary(w http.ResponseWriter, r *http.Request) { + locale := store.LocaleFromContext(r.Context()) + var req struct { + BinaryName string `json:"binary_name"` + } + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, 1<<20)).Decode(&req); err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidJSON)}) + return + } + if req.BinaryName == "" { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgRequired, "binary_name")}) + return + } + // Security: only allow simple binary names (no path separators, no shell metacharacters). + // This prevents probing arbitrary filesystem paths via LookPath. + if !safeBinaryNameRe.MatchString(req.BinaryName) { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid binary name"}) + return + } + absPath, err := exec.LookPath(req.BinaryName) + if err != nil { + writeJSON(w, http.StatusOK, map[string]any{"found": false, "error": fmt.Sprintf("binary %q not found in PATH", req.BinaryName)}) + return + } + writeJSON(w, http.StatusOK, map[string]any{"found": true, "path": absPath}) +} + // dryRunRequest tests commands against deny patterns. type dryRunRequest struct { TestCommands []string `json:"test_commands"` diff --git a/internal/store/pg/secure_cli.go b/internal/store/pg/secure_cli.go index 1d88d4b2..96c882d9 100644 --- a/internal/store/pg/secure_cli.go +++ b/internal/store/pg/secure_cli.go @@ -26,6 +26,11 @@ func NewPGSecureCLIStore(db *sql.DB, encryptionKey string) *PGSecureCLIStore { const secureCLISelectCols = `id, binary_name, binary_path, description, encrypted_env, deny_args, deny_verbose, timeout_seconds, tips, agent_id, enabled, created_by, created_at, updated_at` +// secureCLISelectColsAliased is the same as secureCLISelectCols but prefixed with table alias "b." +// Required for LookupByBinary which uses LEFT JOIN (ambiguous column names without prefix). +const secureCLISelectColsAliased = `b.id, b.binary_name, b.binary_path, b.description, b.encrypted_env, + b.deny_args, b.deny_verbose, b.timeout_seconds, b.tips, b.agent_id, b.enabled, b.created_by, b.created_at, b.updated_at` + func (s *PGSecureCLIStore) Create(ctx context.Context, b *store.SecureCLIBinary) error { if err := store.ValidateUserID(b.CreatedBy); err != nil { return err @@ -271,7 +276,8 @@ func (s *PGSecureCLIStore) LookupByBinary(ctx context.Context, binaryName string } // Build query with optional LEFT JOIN for per-user credentials. - selectCols := secureCLISelectCols + // Use aliased columns (b.) to avoid ambiguous column reference with JOIN. + selectCols := secureCLISelectColsAliased joinClause := "" if userID != "" { selectCols += ", uc.encrypted_env AS user_env" diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 1fb41363..7d818d48 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -281,6 +281,7 @@ func formatCredentialedResult(binary string, args []string, // Returns the credential config and parsed args, or nil if not credentialed. func (t *ExecTool) lookupCredentialedBinary(ctx context.Context, command string) (*store.SecureCLIBinary, string, []string) { if t.secureCLIStore == nil { + slog.Warn("secure_cli.lookup: store is nil, skipping credentialed exec", "command", command) return nil, "", nil } binary, args, err := parseCommandBinary(command) @@ -296,9 +297,15 @@ func (t *ExecTool) lookupCredentialedBinary(ctx context.Context, command string) // Pass userID for per-user credential resolution (LEFT JOIN, zero extra queries). userID := store.UserIDFromContext(ctx) cred, err := t.secureCLIStore.LookupByBinary(ctx, binary, agentIDPtr, userID) - if err != nil || cred == nil { + if err != nil { + slog.Warn("secure_cli.lookup: query failed", "binary", binary, "agent_id", agentID, "error", err) return nil, "", nil } + if cred == nil { + slog.Warn("secure_cli.lookup: no credential found", "binary", binary, "agent_id", agentID) + return nil, "", nil + } + slog.Info("secure_cli.lookup: found credential", "binary", binary, "cred_id", cred.ID, "env_size", len(cred.EncryptedEnv)) return cred, binary, args } diff --git a/ui/web/src/i18n/locales/en/cli-credentials.json b/ui/web/src/i18n/locales/en/cli-credentials.json index d6a4d214..4a0b0419 100644 --- a/ui/web/src/i18n/locales/en/cli-credentials.json +++ b/ui/web/src/i18n/locales/en/cli-credentials.json @@ -29,7 +29,7 @@ "denyVerbose": "Deny Verbose Args", "timeout": "Timeout (s)", "tips": "Tips", - "agentId": "Agent ID", + "agentId": "Agent", "agentIdHint": "optional — leave blank for global", "commaSeparated": "comma-separated", "binaryNameRequired": "Binary name is required.", @@ -38,16 +38,21 @@ "noEnvVarsHint": "Click \"Add Variable\" to define environment variables for this CLI tool.", "envKeyPlaceholder": "ENV_VAR_NAME", "envValuePlaceholder": "value", - "invalidEnvKey": "\"{{key}}\" is not a valid env variable name (use A-Z, 0-9, underscore)." + "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", + "binaryFound": "Found at {{path}}", + "binaryNotFound": "Binary not found in server PATH", + "checking": "Checking..." }, "placeholders": { "binaryName": "e.g. gh", - "binaryPath": "/usr/local/bin/gh", + "binaryPath": "/usr/local/bin/...", "description": "What this CLI tool does", "denyArgs": "--admin, --force", "denyVerbose": "-v, --verbose", "tips": "Usage tips for the agent", - "agentId": "agent-id" + "agentId": "Global (all agents)" }, "toast": { "created": "CLI credential created", diff --git a/ui/web/src/i18n/locales/vi/cli-credentials.json b/ui/web/src/i18n/locales/vi/cli-credentials.json index 58ef1e70..056140e8 100644 --- a/ui/web/src/i18n/locales/vi/cli-credentials.json +++ b/ui/web/src/i18n/locales/vi/cli-credentials.json @@ -29,7 +29,7 @@ "denyVerbose": "Verbose Args bị chặn", "timeout": "Thời gian chờ (s)", "tips": "Mẹo sử dụng", - "agentId": "ID Agent", + "agentId": "Agent", "agentIdHint": "tùy chọn — để trống cho toàn cục", "commaSeparated": "phân cách bằng dấu phẩy", "binaryNameRequired": "Tên binary là bắt buộc.", @@ -38,16 +38,21 @@ "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ị", - "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)." + "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", + "binaryFound": "Tìm thấy tại {{path}}", + "binaryNotFound": "Không tìm thấy binary trên server", + "checking": "Đang kiểm tra..." }, "placeholders": { "binaryName": "vd: gh", - "binaryPath": "/usr/local/bin/gh", + "binaryPath": "/usr/local/bin/...", "description": "CLI tool này dùng để làm gì", "denyArgs": "--admin, --force", "denyVerbose": "-v, --verbose", "tips": "Mẹo sử dụng cho agent", - "agentId": "agent-id" + "agentId": "Toàn cục (tất cả agent)" }, "toast": { "created": "Đã tạo thông tin CLI", diff --git a/ui/web/src/i18n/locales/zh/cli-credentials.json b/ui/web/src/i18n/locales/zh/cli-credentials.json index 4d33ed00..59beadc0 100644 --- a/ui/web/src/i18n/locales/zh/cli-credentials.json +++ b/ui/web/src/i18n/locales/zh/cli-credentials.json @@ -29,7 +29,7 @@ "denyVerbose": "禁止详细参数", "timeout": "超时 (秒)", "tips": "提示", - "agentId": "代理 ID", + "agentId": "代理", "agentIdHint": "可选 — 留空表示全局", "commaSeparated": "逗号分隔", "binaryNameRequired": "二进制名称为必填项。", @@ -38,16 +38,21 @@ "noEnvVarsHint": "点击\"添加变量\"为此 CLI 工具定义环境变量。", "envKeyPlaceholder": "变量名", "envValuePlaceholder": "值", - "invalidEnvKey": "\"{{key}}\" 不是有效的环境变量名（使用 A-Z、0-9、下划线）。" + "invalidEnvKey": "\"{{key}}\" 不是有效的环境变量名（使用 A-Z、0-9、下划线）。", + "binaryPathHint": "留空自动从 PATH 检测", + "checkBinary": "检查", + "binaryFound": "已找到：{{path}}", + "binaryNotFound": "在服务器 PATH 中未找到", + "checking": "检查中..." }, "placeholders": { "binaryName": "例如 gh", - "binaryPath": "/usr/local/bin/gh", + "binaryPath": "/usr/local/bin/...", "description": "此 CLI 工具的用途", "denyArgs": "--admin, --force", "denyVerbose": "-v, --verbose", "tips": "给代理的使用提示", - "agentId": "agent-id" + "agentId": "全局（所有代理）" }, "toast": { "created": "CLI 凭证已创建", 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 27ac2821..1de83fba 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 @@ -11,8 +11,9 @@ import { Switch } from "@/components/ui/switch"; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue, } from "@/components/ui/select"; -import { Plus, X } from "lucide-react"; +import { Plus, X, Search, Check, AlertCircle } from "lucide-react"; import { useHttp } from "@/hooks/use-ws"; +import { useAgents } from "@/pages/agents/hooks/use-agents"; import type { SecureCLIBinary, CLICredentialInput, CLIPreset } from "./hooks/use-cli-credentials"; interface ManualEnvEntry { @@ -29,11 +30,13 @@ interface Props { } const NONE_PRESET = "__none__"; +const GLOBAL_AGENT = "__global__"; export function CliCredentialFormDialog({ open, onOpenChange, credential, presets, onSubmit }: Props) { const { t } = useTranslation("cli-credentials"); const { t: tc } = useTranslation("common"); const http = useHttp(); + const { agents } = useAgents(); const [selectedPreset, setSelectedPreset] = useState(NONE_PRESET); const [binaryName, setBinaryName] = useState(""); @@ -52,6 +55,10 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset const [loading, setLoading] = useState(false); const [error, setError] = useState(""); + // Check binary state + const [checking, setChecking] = useState(false); + const [checkResult, setCheckResult] = useState<{ found: boolean; path?: string; error?: string } | null>(null); + const isEdit = !!credential; const presetEntries: Array<[string, CLIPreset]> = Object.entries(presets).filter( (e): e is [string, CLIPreset] => e[1] !== undefined, @@ -76,6 +83,7 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset setEnabled(credential?.enabled ?? true); setEnvValues({}); setError(""); + setCheckResult(null); if (!credential) { setInitialEnvKeys([]); @@ -123,6 +131,27 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset setManualEnvEntries([]); }; + const handleCheckBinary = async () => { + const name = binaryName.trim(); + if (!name) return; + setChecking(true); + setCheckResult(null); + try { + const res = await http.post<{ found: boolean; path?: string; error?: string }>( + "/v1/cli-credentials/check-binary", + { binary_name: name }, + ); + setCheckResult(res); + if (res.found && res.path) { + setBinaryPath(res.path); + } + } catch { + setCheckResult({ found: false, error: t("form.binaryNotFound") }); + } finally { + setChecking(false); + } + }; + const addManualEnvEntry = useCallback(() => { setManualEnvEntries((prev) => [...prev, { key: "", value: "" }]); }, []); @@ -304,16 +333,37 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset )} + {/* Binary name + check button */}
- setBinaryName(e.target.value)} - placeholder={t("placeholders.binaryName")} - className="text-base md:text-sm" - /> +
+ { setBinaryName(e.target.value); setCheckResult(null); }} + placeholder={t("placeholders.binaryName")} + className="text-base md:text-sm" + /> + +
+ {checkResult && ( +

+ {checkResult.found ? : } + {checkResult.found ? t("form.binaryFound", { path: checkResult.path }) : (checkResult.error || t("form.binaryNotFound"))} +

+ )} + {checking &&

{t("form.checking")}

}
@@ -324,6 +374,7 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset placeholder={t("placeholders.binaryPath")} className="text-base md:text-sm" /> +

{t("form.binaryPathHint")}

@@ -386,15 +437,22 @@ export function CliCredentialFormDialog({ open, onOpenChange, credential, preset /> + {/* Agent selector */}
- - setAgentId(e.target.value)} - placeholder={t("placeholders.agentId")} - className="text-base md:text-sm" - /> + +