From c4e0bd2be51dd2fc45b54bbb2dad36f999aa1571 Mon Sep 17 00:00:00 2001 From: Duy /zuey/ Date: Sun, 31 May 2026 11:05:47 +0700 Subject: [PATCH] fix(cli-credentials): stabilize git preset creation Squash merge PR #96 after PR #92. PR checks were green; dev beta CI will be monitored after merge. --- docs/project-changelog.md | 24 ++++++ internal/http/secure_cli.go | 75 ++++++++++++++++--- internal/http/secure_cli_check_binary_test.go | 55 ++++++++++++++ internal/http/secure_cli_env_response_test.go | 11 ++- .../__tests__/use-cli-credentials.test.ts | 33 ++++++++ .../cli-credential-env-vars-section.tsx | 6 +- .../hooks/use-cli-credentials.ts | 30 +++++++- ui/web/src/types/cli-credential.ts | 1 + 8 files changed, 216 insertions(+), 19 deletions(-) create mode 100644 ui/web/src/pages/cli-credentials/__tests__/use-cli-credentials.test.ts diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 1027b40d..75f99fe8 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -4,6 +4,30 @@ Significant changes, features, and fixes in reverse chronological order. --- +## 2026-05-29 + +### CLI Credentials git preset null-env crash (issue #93) + +**Fixes** + +- Stopped `/v1/cli-credentials/presets` from returning nullable preset arrays + for adapter-managed CLIs such as `git`; frontend now also normalizes older + `null` payloads defensively. +- Allowed the `git` preset to create a SecureCLI binary without legacy env + vars and persisted its `adapter_name=git`, so the PAT/SSH user credential + flow activates after creation. +- Guarded the preset env-var renderer against nullish `env_vars` to keep the + Runtime & Packages → CLI Credentials tab renderable. + +**Tests** + +- Added backend regression coverage for stable preset arrays and git preset + creation without legacy env. +- Added frontend normalization coverage for nullable adapter-managed preset + arrays. + +--- + ## 2026-05-28 ### CLI credential adapter framework + git adapter (issue #82) diff --git a/internal/http/secure_cli.go b/internal/http/secure_cli.go index ee4d0ad8..1e3d51ba 100644 --- a/internal/http/secure_cli.go +++ b/internal/http/secure_cli.go @@ -116,9 +116,10 @@ func (h *SecureCLIHandler) handleCreate(w http.ResponseWriter, r *http.Request) return } + var preset *tools.CLIPreset // Apply preset defaults if specified if req.Preset != "" { - preset := tools.GetPreset(req.Preset) + preset = tools.GetPreset(req.Preset) if preset == nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "unknown preset: " + req.Preset}) return @@ -147,22 +148,31 @@ func (h *SecureCLIHandler) handleCreate(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgRequired, "binary_name")}) return } - if len(req.Env) == 0 { + adapterManagedPreset := preset != nil && strings.TrimSpace(preset.AdapterName) != "" && len(preset.EnvVars) == 0 + if len(req.Env) == 0 && !adapterManagedPreset { writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgRequired, "env")}) return } - envEntries, err := store.ParseSecureCLIEnv(req.Env) - if err != nil { - 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 + envJSON := []byte("{}") + if len(req.Env) > 0 { + envEntries, err := store.ParseSecureCLIEnv(req.Env) + if err != nil { + 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 + } } + var adapterName *string + if preset != nil && strings.TrimSpace(preset.AdapterName) != "" { + name := strings.TrimSpace(preset.AdapterName) + adapterName = &name + } b := &store.SecureCLIBinary{ BinaryName: req.BinaryName, BinaryPath: req.BinaryPath, @@ -175,6 +185,7 @@ func (h *SecureCLIHandler) handleCreate(w http.ResponseWriter, r *http.Request) IsGlobal: req.IsGlobal == nil || *req.IsGlobal, // default true Enabled: req.Enabled, CreatedBy: store.UserIDFromContext(r.Context()), + AdapterName: adapterName, } if b.TimeoutSeconds <= 0 { b.TimeoutSeconds = 30 @@ -290,8 +301,48 @@ func (h *SecureCLIHandler) handleDelete(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, map[string]string{"status": "deleted"}) } +type cliPresetResponse struct { + BinaryName string `json:"binary_name"` + Description string `json:"description"` + EnvVars []tools.EnvVarDef `json:"env_vars"` + DenyArgs []string `json:"deny_args"` + DenyVerbose []string `json:"deny_verbose"` + Timeout int `json:"timeout"` + Tips string `json:"tips"` + AdapterName string `json:"adapter_name,omitempty"` +} + +func normalizedCLIPresets() map[string]cliPresetResponse { + out := make(map[string]cliPresetResponse, len(tools.CLIPresets)) + for key, preset := range tools.CLIPresets { + envVars := preset.EnvVars + if envVars == nil { + envVars = []tools.EnvVarDef{} + } + denyArgs := preset.DenyArgs + if denyArgs == nil { + denyArgs = []string{} + } + denyVerbose := preset.DenyVerbose + if denyVerbose == nil { + denyVerbose = []string{} + } + out[key] = cliPresetResponse{ + BinaryName: preset.BinaryName, + Description: preset.Description, + EnvVars: envVars, + DenyArgs: denyArgs, + DenyVerbose: denyVerbose, + Timeout: preset.Timeout, + Tips: preset.Tips, + AdapterName: preset.AdapterName, + } + } + return out +} + func (h *SecureCLIHandler) handlePresets(w http.ResponseWriter, _ *http.Request) { - writeJSON(w, http.StatusOK, map[string]any{"presets": tools.CLIPresets}) + writeJSON(w, http.StatusOK, map[string]any{"presets": normalizedCLIPresets()}) } // handleCheckBinary resolves a binary name to its absolute path via exec.LookPath. diff --git a/internal/http/secure_cli_check_binary_test.go b/internal/http/secure_cli_check_binary_test.go index 3bf0fc1f..a9f41b8f 100644 --- a/internal/http/secure_cli_check_binary_test.go +++ b/internal/http/secure_cli_check_binary_test.go @@ -110,6 +110,61 @@ func TestSecureCLIPresetsIncludesGoogleWorkspace(t *testing.T) { } } +func TestSecureCLIPresetsReturnStableArraysForGit(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/v1/cli-credentials/presets", nil) + rec := httptest.NewRecorder() + + NewSecureCLIHandler(nil, nil).handlePresets(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + var got struct { + Presets map[string]struct { + EnvVars []struct{} `json:"env_vars"` + DenyVerbose []string `json:"deny_verbose"` + AdapterName string `json:"adapter_name"` + } `json:"presets"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + git := got.Presets["git"] + if git.EnvVars == nil { + t.Fatalf("git env_vars = nil; response must use [] for frontend compatibility") + } + if git.DenyVerbose == nil { + t.Fatalf("git deny_verbose = nil; response must use [] for frontend compatibility") + } + if git.AdapterName != "git" { + t.Fatalf("git adapter_name = %q, want git", git.AdapterName) + } +} + +func TestSecureCLICreateAllowsGitPresetWithoutLegacyEnv(t *testing.T) { + st := &fakeSecureCLIStore{} + req := httptest.NewRequest(http.MethodPost, "/v1/cli-credentials", strings.NewReader(`{"preset":"git"}`)) + rec := httptest.NewRecorder() + + NewSecureCLIHandler(st, nil).handleCreate(rec, req) + + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", rec.Code, rec.Body.String()) + } + if st.created == nil { + t.Fatal("expected store.Create to be called") + } + if st.created.BinaryName != "git" { + t.Fatalf("binary_name = %q, want git", st.created.BinaryName) + } + if st.created.AdapterName == nil || *st.created.AdapterName != "git" { + t.Fatalf("adapter_name = %#v, want git", st.created.AdapterName) + } + if string(st.created.EncryptedEnv) != "{}" { + t.Fatalf("encrypted env = %q, want {}", st.created.EncryptedEnv) + } +} + func TestSecureCLICheckBinaryFindsNpmPackageCliAlias(t *testing.T) { runtimeDir := t.TempDir() t.Setenv("RUNTIME_DIR", runtimeDir) diff --git a/internal/http/secure_cli_env_response_test.go b/internal/http/secure_cli_env_response_test.go index be2552a4..5bbad3ff 100644 --- a/internal/http/secure_cli_env_response_test.go +++ b/internal/http/secure_cli_env_response_test.go @@ -14,11 +14,16 @@ import ( ) type fakeSecureCLIStore struct { - binary *store.SecureCLIBinary - user *store.SecureCLIUserCredential + binary *store.SecureCLIBinary + user *store.SecureCLIUserCredential + created *store.SecureCLIBinary } -func (s *fakeSecureCLIStore) Create(context.Context, *store.SecureCLIBinary) error { return nil } +func (s *fakeSecureCLIStore) Create(_ context.Context, b *store.SecureCLIBinary) error { + cp := *b + s.created = &cp + return nil +} func (s *fakeSecureCLIStore) Get(context.Context, uuid.UUID) (*store.SecureCLIBinary, error) { cp := *s.binary return &cp, nil diff --git a/ui/web/src/pages/cli-credentials/__tests__/use-cli-credentials.test.ts b/ui/web/src/pages/cli-credentials/__tests__/use-cli-credentials.test.ts new file mode 100644 index 00000000..ea3349cd --- /dev/null +++ b/ui/web/src/pages/cli-credentials/__tests__/use-cli-credentials.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from "vitest"; +import { normalizeCliPreset, normalizeCliPresets } from "../hooks/use-cli-credentials"; + +describe("cli credential preset normalization", () => { + it("normalizes nullable arrays from adapter-managed presets", () => { + const preset = normalizeCliPreset({ + binary_name: "git", + description: "Git with credential adapter", + env_vars: null, + deny_args: ["(?i)credential-helper"], + deny_verbose: null, + timeout: 300, + tips: "Adapter handles auth automatically", + adapter_name: "git", + }); + + expect(preset).toEqual({ + binary_name: "git", + description: "Git with credential adapter", + env_vars: [], + deny_args: ["(?i)credential-helper"], + deny_verbose: [], + timeout: 300, + tips: "Adapter handles auth automatically", + adapter_name: "git", + }); + }); + + it("returns an empty preset map for missing API payloads", () => { + expect(normalizeCliPresets(null)).toEqual({}); + expect(normalizeCliPresets(undefined)).toEqual({}); + }); +}); 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 528bf6fe..190e5e5f 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 @@ -53,12 +53,14 @@ export function CliCredentialEnvVarsSection({ ); }, [setManualEnvEntries]); + const presetEnvVars = activePreset?.env_vars ?? []; + // Preset-driven env var inputs - if (!isManualMode && activePreset && activePreset.env_vars.length > 0) { + if (!isManualMode && presetEnvVars.length > 0) { return (

{t("form.envVars")}

- {activePreset.env_vars.map((ev) => ( + {presetEnvVars.map((ev) => (