From 02d7b6f3de763ba401f6553f76aed3e46583d239 Mon Sep 17 00:00:00 2001 From: Goon Date: Sun, 31 May 2026 19:26:35 +0700 Subject: [PATCH] fix: stabilize agent git access --- docs/git-credential-adapter.md | 19 +- docs/project-changelog.md | 19 ++ internal/http/secure_cli_agent_credentials.go | 2 +- internal/http/secure_cli_typed_credentials.go | 5 +- internal/http/secure_cli_user_credentials.go | 2 +- internal/tools/credential_adapter_git.go | 8 +- .../credential_adapter_git_keyvalidate.go | 41 ++++ .../tools/credential_adapter_git_ssh_test.go | 27 ++- internal/tools/credential_adapter_git_test.go | 14 +- .../phase-01-characterization-tests.md | 33 +++ .../phase-02-agent-access-ui.md | 36 ++++ .../phase-03-git-pat-auth.md | 28 +++ .../phase-04-ssh-key-validation.md | 34 +++ .../phase-05-docs-ship.md | 33 +++ .../plan.md | 81 ++++++++ .../reports/redteam-report.md | 30 +++ .../reports/validation-report.md | 18 ++ tests/integration/git_adapter_pat_test.go | 2 +- tests/integration/githttp_helper_test.go | 8 +- .../src/i18n/locales/en/cli-credentials.json | 6 + .../src/i18n/locales/vi/cli-credentials.json | 6 + .../src/i18n/locales/zh/cli-credentials.json | 6 + .../cli-agent-credentials-wiring.test.ts | 19 +- .../cli-agent-access-dialog.tsx | 69 +++++++ .../cli-agent-credentials-content.tsx | 193 ++++++++++++++++++ .../cli-agent-credentials-dialog.tsx | 180 +--------------- .../cli-credential-grants-content.tsx | 151 ++++++++++++++ .../cli-credential-grants-dialog.tsx | 151 +------------- .../cli-credentials/cli-credentials-panel.tsx | 39 ++-- .../cli-credentials/cli-credentials-table.tsx | 20 +- 30 files changed, 895 insertions(+), 385 deletions(-) create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-01-characterization-tests.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-02-agent-access-ui.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-03-git-pat-auth.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-04-ssh-key-validation.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-05-docs-ship.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/plan.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/redteam-report.md create mode 100644 plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/validation-report.md create mode 100644 ui/web/src/pages/cli-credentials/cli-agent-access-dialog.tsx create mode 100644 ui/web/src/pages/cli-credentials/cli-agent-credentials-content.tsx create mode 100644 ui/web/src/pages/cli-credentials/cli-credential-grants-content.tsx diff --git a/docs/git-credential-adapter.md b/docs/git-credential-adapter.md index 25715c1f..c45e64cf 100644 --- a/docs/git-credential-adapter.md +++ b/docs/git-credential-adapter.md @@ -31,8 +31,8 @@ ID ambiguity: the selected agent owns the credential, and anyone allowed to use that agent can cause it to run git with the stored credential. 1. Open **Packages → CLI Credentials**. -2. Pick the `git` row and open **Agent Credentials**. -3. Select the agent. +2. Pick the `git` row and open **Agent Access**. +3. Use the **Credential** tab to select the agent. 4. Choose **Credential Type**: `Personal Access Token` or `SSH Private Key`. 5. Enter **Host Scope** (required for PAT/SSH): the hostname the credential authenticates to. @@ -42,6 +42,11 @@ that agent can cause it to run git with the stored credential. 6. Paste the token (PAT) or the unencrypted PEM body (SSH). 7. Save. +Use the **Access policy** tab in the same Agent Access dialog when you need to +change deny args, timeout, tips, or env overrides for that agent. Agent Access +is one dialog on purpose: policy and secret storage stay separate internally, +but operators should manage them as one access decision. + ## Advanced user overrides Per-user credentials remain available for personal overrides and backward @@ -117,9 +122,11 @@ the operation with a 401/403). - Injected via `GIT_CONFIG_COUNT` + `GIT_CONFIG_KEY_*` / `GIT_CONFIG_VALUE_*` environment variables. - The PAT itself goes into a value that synthesizes an `http..extraheader` - config entry with `AUTHORIZATION: basic `. + config entry with `Authorization: Basic base64("x-access-token:")`. - **The PAT never appears on argv** — so `ps`, `/proc//cmdline`, and shell-history echoes don't expose it. +- The raw PAT, base64 payload, and full injected header are all registered with + the scrubber before tool output is returned to the agent. - The injected env vars are scoped to the spawned `git` process only; they are NOT inherited by goclaw, by other tools, or by sibling exec calls. @@ -128,9 +135,13 @@ the operation with a 401/403). - The PEM key is written to an `0600`-mode tmpfile in `os.TempDir()` (per-user on POSIX) with a `goclaw-gitkey-*` prefix. - `GIT_SSH_COMMAND` is set to - `ssh -i -F /dev/null -o IdentitiesOnly=yes -o StrictHostKeyChecking=accept-new`. + `ssh -i -o IdentitiesOnly=yes -o BatchMode=yes -o StrictHostKeyChecking=accept-new`. - The tmpfile is removed via `defer` on the exec wrapper. **SIGKILL of goclaw leaves the file orphaned** — see the Operator Notes section below. +- SSH private keys are validated twice at save time: first with Go's SSH parser, + then with OpenSSH via `ssh-keygen -y -f ` when `ssh-keygen` is + available. This catches keys that would otherwise save successfully but fail + later with OpenSSH diagnostics such as `error in libcrypto`. - `StrictHostKeyChecking=accept-new` accepts unknown host keys on first contact (TOFU). A network attacker positioned between goclaw and the git host CAN capture the SSH session on the first connection. Operators should diff --git a/docs/project-changelog.md b/docs/project-changelog.md index fe0bfbd5..55e1b23e 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,25 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-31 +### Agent Access git credential follow-up (issue #117) + +**Fixes** + +- Replaced separate Agent Grants and Agent Credentials row actions with one + Agent Access dialog containing Credential and Access policy tabs, preventing + overlapping agent-access modals. +- Git PAT credentials now inject GitHub-compatible Basic auth extraheaders + instead of Bearer headers. +- SSH private keys are now checked with OpenSSH at save time when `ssh-keygen` + is available, catching keys that would later fail with `error in libcrypto`. + +**Security** + +- Git PAT redaction now includes the raw token, the base64 Basic auth payload, + and the full injected header value. + +--- + ### Tool-call announcements **Fixes** diff --git a/internal/http/secure_cli_agent_credentials.go b/internal/http/secure_cli_agent_credentials.go index ebe4115a..c7d0001f 100644 --- a/internal/http/secure_cli_agent_credentials.go +++ b/internal/http/secure_cli_agent_credentials.go @@ -161,7 +161,7 @@ func (h *SecureCLIHandler) handleSetAgentCredentials(w http.ResponseWriter, r *h if !bindJSON(w, r, locale, &body) { return } - envBytes, credType, hostScope, terr := prepareTypedCredentialEnv(locale, body.typedCredentialBody) + envBytes, credType, hostScope, terr := prepareTypedCredentialEnv(r.Context(), locale, body.typedCredentialBody) if terr != nil { writeTypedCredentialError(w, terr) return diff --git a/internal/http/secure_cli_typed_credentials.go b/internal/http/secure_cli_typed_credentials.go index 48f31cae..92527129 100644 --- a/internal/http/secure_cli_typed_credentials.go +++ b/internal/http/secure_cli_typed_credentials.go @@ -8,6 +8,7 @@ package http import ( + "context" "encoding/json" "errors" "fmt" @@ -55,7 +56,7 @@ func (e *errTypedCredential) Error() string { return e.msg } // the bytes that get encrypted-and-stored. Returns (envBytes, credentialType, // hostScope, error). On success, envBytes is the JSON-encoded blob (e.g. // `{"token":"..."}` or `{"key":"..."}`) ready for SetUserCredentialsTyped. -func prepareTypedCredentialEnv(locale string, body typedCredentialBody) ([]byte, *string, *string, *errTypedCredential) { +func prepareTypedCredentialEnv(ctx context.Context, locale string, body typedCredentialBody) ([]byte, *string, *string, *errTypedCredential) { if body.CredentialType == nil || *body.CredentialType == "" || *body.CredentialType == "env" { // Caller should route to legacy env path. Indicate that with all-nil // returns + nil error; handler treats this as "fall through". @@ -126,7 +127,7 @@ func prepareTypedCredentialEnv(locale string, body typedCredentialBody) ([]byte, errorKey: "git.cred_blob_missing_key", } } - if err := tools.ValidateSSHKey([]byte(key)); err != nil { + if err := tools.ValidateSSHKeyForStorage(ctx, []byte(key)); err != nil { if errors.Is(err, tools.ErrSSHKeyPassphraseUnsupported) { return nil, nil, nil, &errTypedCredential{ status: http.StatusBadRequest, diff --git a/internal/http/secure_cli_user_credentials.go b/internal/http/secure_cli_user_credentials.go index f4f0a428..33a5fcae 100644 --- a/internal/http/secure_cli_user_credentials.go +++ b/internal/http/secure_cli_user_credentials.go @@ -127,7 +127,7 @@ func (h *SecureCLIHandler) handleSetUserCredentials(w http.ResponseWriter, r *ht // credential_type + host_scope. Audit emits credential_type only — never // the secret or host (host is operator-visible config but still scoped to // the audit channel). - envBytes, credType, hostScope, terr := prepareTypedCredentialEnv(locale, body.typedCredentialBody) + envBytes, credType, hostScope, terr := prepareTypedCredentialEnv(r.Context(), locale, body.typedCredentialBody) if terr != nil { writeTypedCredentialError(w, terr) return diff --git a/internal/tools/credential_adapter_git.go b/internal/tools/credential_adapter_git.go index 5e4a9fc6..2ab167b6 100644 --- a/internal/tools/credential_adapter_git.go +++ b/internal/tools/credential_adapter_git.go @@ -2,6 +2,7 @@ package tools import ( "context" + "encoding/base64" "encoding/json" "errors" "fmt" @@ -115,19 +116,20 @@ func (gitAdapter) Prepare(ctx context.Context, _ *store.SecureCLIBinary, cred *s return nil, err } // GIT_CONFIG_COUNT env approach (git 2.31+) — same effect as - // `-c http.https://host/.extraheader=Authorization: Bearer ` but + // `-c http.https://host/.extraheader=Authorization: Basic ...` but // the token stays out of argv. Single-entry header keeps host-scoping // strict; the matching url prefix ensures git skips its credential // helpers for this URL automatically. configKey := fmt.Sprintf("http.https://%s/.extraheader", targetHost) - configVal := fmt.Sprintf("Authorization: Bearer %s", token) + basicPayload := base64.StdEncoding.EncodeToString([]byte("x-access-token:" + token)) + configVal := "Authorization: Basic " + basicPayload return &Injection{ Env: map[string]string{ "GIT_CONFIG_COUNT": "1", "GIT_CONFIG_KEY_0": configKey, "GIT_CONFIG_VALUE_0": configVal, }, - ScrubValues: []string{token}, + ScrubValues: []string{token, basicPayload, configVal}, }, nil case "ssh_key": keyPEM, err := decodeSSHKeyBlob(cred.EncryptedEnv) diff --git a/internal/tools/credential_adapter_git_keyvalidate.go b/internal/tools/credential_adapter_git_keyvalidate.go index 7a28652d..43a6af4a 100644 --- a/internal/tools/credential_adapter_git_keyvalidate.go +++ b/internal/tools/credential_adapter_git_keyvalidate.go @@ -13,8 +13,11 @@ package tools // can strip the passphrase via `ssh-keygen -p` before saving. import ( + "context" "errors" "fmt" + "os/exec" + "strings" "golang.org/x/crypto/ssh" ) @@ -30,6 +33,44 @@ var ErrSSHKeyPassphraseUnsupported = errors.New("ssh key: passphrase-protected k // DB write or audit emit. Phase 3's runtime check still defends in depth. func ValidatePATToken(tok string) error { return validateTokenShape(tok) } +// ValidateSSHKeyForStorage runs the normal parser plus an OpenSSH parser check. +// The second check catches keys that Go accepts but the runtime ssh client would +// reject later with opaque errors such as "error in libcrypto". +func ValidateSSHKeyForStorage(ctx context.Context, pem []byte) error { + if err := ValidateSSHKey(pem); err != nil { + return err + } + if ctx == nil { + ctx = context.Background() + } + return openSSHKeyCompatibilityCheck(ctx, pem) +} + +var openSSHKeyCompatibilityCheck = validateSSHKeyOpenSSHCompatible + +func validateSSHKeyOpenSSHCompatible(ctx context.Context, pem []byte) error { + sshKeygen, err := exec.LookPath("ssh-keygen") + if err != nil { + return nil + } + keyPath, cleanup, err := materializeEphemeral(ctx, pem, "gitkey-validate") + if err != nil { + return err + } + defer func() { _ = cleanup() }() + + out, err := exec.CommandContext(ctx, sshKeygen, "-y", "-f", keyPath).CombinedOutput() + if err == nil { + return nil + } + msg := strings.TrimSpace(string(out)) + if msg == "" { + msg = err.Error() + } + msg = strings.ReplaceAll(msg, keyPath, "[keyfile]") + return fmt.Errorf("ssh key openssh parse: %s", msg) +} + // ValidateSSHKey parses the supplied PEM with x/crypto/ssh. Any non-nil // error blocks the save: a key that fails to parse here will also fail at // `ssh -i` time, and we want the diagnostic to happen at save (where the diff --git a/internal/tools/credential_adapter_git_ssh_test.go b/internal/tools/credential_adapter_git_ssh_test.go index 3039322d..d0d7563e 100644 --- a/internal/tools/credential_adapter_git_ssh_test.go +++ b/internal/tools/credential_adapter_git_ssh_test.go @@ -58,10 +58,7 @@ func base64Encode(b []byte) string { s := base64.StdEncoding.EncodeToString(b) var out strings.Builder for i := 0; i < len(s); i += 70 { - end := i + 70 - if end > len(s) { - end = len(s) - } + end := min(i+70, len(s)) if i > 0 { out.WriteByte('\n') } @@ -86,6 +83,28 @@ func TestValidateSSHKey_AcceptsUnencrypted(t *testing.T) { } } +func TestValidateSSHKeyForStorage_UsesOpenSSHCompatibilityCheck(t *testing.T) { + original := openSSHKeyCompatibilityCheck + t.Cleanup(func() { openSSHKeyCompatibilityCheck = original }) + + called := false + openSSHKeyCompatibilityCheck = func(context.Context, []byte) error { + called = true + return errors.New("ssh-keygen: error in libcrypto") + } + + err := ValidateSSHKeyForStorage(context.Background(), genEd25519PEM(t)) + if err == nil { + t.Fatal("expected OpenSSH compatibility error") + } + if !called { + t.Fatal("OpenSSH compatibility check was not called") + } + if !strings.Contains(err.Error(), "libcrypto") { + t.Fatalf("expected OpenSSH diagnostic, got %v", err) + } +} + // 3. Garbage rejected without exposing as passphrase. func TestValidateSSHKey_RejectsGarbage(t *testing.T) { err := ValidateSSHKey([]byte("not a key")) diff --git a/internal/tools/credential_adapter_git_test.go b/internal/tools/credential_adapter_git_test.go index bb5a6b9c..e4bb5068 100644 --- a/internal/tools/credential_adapter_git_test.go +++ b/internal/tools/credential_adapter_git_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "encoding/base64" "encoding/json" "errors" "os" @@ -9,6 +10,7 @@ import ( "path/filepath" "reflect" "runtime" + "slices" "sort" "strings" "testing" @@ -181,13 +183,19 @@ func TestGitAdapter_PreparePAT(t *testing.T) { wantEnv := map[string]string{ "GIT_CONFIG_COUNT": "1", "GIT_CONFIG_KEY_0": "http.https://github.com/.extraheader", - "GIT_CONFIG_VALUE_0": "Authorization: Bearer ghp_abc", + "GIT_CONFIG_VALUE_0": "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte("x-access-token:ghp_abc")), } if !reflect.DeepEqual(inj.Env, wantEnv) { t.Errorf("Env=%v, want %v", inj.Env, wantEnv) } - if len(inj.ScrubValues) != 1 || inj.ScrubValues[0] != "ghp_abc" { - t.Errorf("ScrubValues=%v, want [ghp_abc]", inj.ScrubValues) + for _, secret := range []string{ + "ghp_abc", + base64.StdEncoding.EncodeToString([]byte("x-access-token:ghp_abc")), + wantEnv["GIT_CONFIG_VALUE_0"], + } { + if !slices.Contains(inj.ScrubValues, secret) { + t.Errorf("ScrubValues=%v missing %q", inj.ScrubValues, secret) + } } if inj.Cleanup != nil { t.Errorf("Cleanup must be nil for PAT path") diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-01-characterization-tests.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-01-characterization-tests.md new file mode 100644 index 00000000..8733d58a --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-01-characterization-tests.md @@ -0,0 +1,33 @@ +--- +phase: 1 +title: characterization-tests +status: completed +effort: S +--- + +# Phase 1: characterization-tests + +## Overview + +Write tests that describe the broken behavior before implementation. + +## Implementation Steps + +1. Add a Web UI wiring test proving `CliCredentialsPanel` exposes one agent + access surface instead of mounting separate Agent Grants and Agent + Credentials dialogs from independent state. +2. Update git adapter PAT tests to expect GitHub-compatible Basic auth + extraheader and to assert both raw and encoded token material are scrubbed. +3. Add SSH storage-validation tests proving save-time validation calls an + OpenSSH compatibility check after Go parser validation. +4. Run the new tests first and keep the initial failing output in the + implementation notes. + +## Success Criteria + +- [ ] UI test fails on current code because two agent dialog targets are still + independent. +- [ ] PAT test fails on current code because it still emits Bearer auth. +- [ ] SSH validation test fails on current code because no OpenSSH + compatibility check is called at storage time. +- [ ] No production secrets or trace payload content are copied into tests. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-02-agent-access-ui.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-02-agent-access-ui.md new file mode 100644 index 00000000..93ff85e9 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-02-agent-access-ui.md @@ -0,0 +1,36 @@ +--- +phase: 2 +title: agent-access-ui +status: completed +effort: M +--- + +# Phase 2: agent-access-ui + +## Overview + +Unify the operator task without merging policy and secret data models. +Admins should open one Agent Access dialog for a binary, then choose between +Credential and Access Policy inside that dialog. + +## Implementation Steps + +1. Replace `agentCredsTarget` and `grantsTarget` in + `cli-credentials-panel.tsx` with one `agentAccessTarget` containing the + binary and initial tab. +2. Add `cli-agent-access-dialog.tsx` as the single dialog root. +3. Extract non-dialog content from `CLIAgentCredentialsDialog` and + `CliCredentialGrantsDialog` into reusable content components so the new + dialog does not nest Radix dialogs. +4. Change table and grant chip actions to open Agent Access with either the + credential or grants tab selected. +5. Add i18n keys for the Agent Access title, description, and tab labels in + English, Vietnamese, and Chinese. + +## Success Criteria + +- [ ] At most one Radix dialog root is mounted for agent access from + `CliCredentialsPanel`. +- [ ] The git row makes typed credential setup the primary Agent Access path. +- [ ] The grants view remains reachable from the same dialog. +- [ ] Existing User Credentials advanced override remains available. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-03-git-pat-auth.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-03-git-pat-auth.md new file mode 100644 index 00000000..6de71d48 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-03-git-pat-auth.md @@ -0,0 +1,28 @@ +--- +phase: 3 +title: git-pat-auth +status: completed +effort: S +--- + +# Phase 3: git-pat-auth + +## Overview + +Fix HTTPS git auth for GitHub PAT credentials. + +## Implementation Steps + +1. In `internal/tools/credential_adapter_git.go`, encode PAT credentials as + `Authorization: Basic base64("x-access-token:")` for + `http.https:///.extraheader`. +2. Preserve host-scoped injection and existing command allowlist behavior. +3. Add both raw token and base64/header forms to scrub values. +4. Update adapter tests and any integration helpers that describe the git HTTP + auth mode. + +## Success Criteria + +- [ ] PAT tests assert Basic auth header, not Bearer. +- [ ] Encoded PAT material cannot appear unsanitized in credentialed exec logs. +- [ ] SSH adapter behavior is unchanged by this phase. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-04-ssh-key-validation.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-04-ssh-key-validation.md new file mode 100644 index 00000000..67119768 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-04-ssh-key-validation.md @@ -0,0 +1,34 @@ +--- +phase: 4 +title: ssh-key-validation +status: completed +effort: M +--- + +# Phase 4: ssh-key-validation + +## Overview + +Catch OpenSSH-incompatible private keys before they are stored. + +## Implementation Steps + +1. Add a storage-oriented validator in `internal/tools` that first calls + `ValidateSSHKey`, then checks OpenSSH compatibility via `ssh-keygen -y -f` + on a 0600 temporary file. +2. If `ssh-keygen` is unavailable, keep the Go parser result rather than + breaking non-OpenSSH dev environments. +3. Route typed credential saves through the new storage validator in + `internal/http/secure_cli_typed_credentials.go`. +4. Return the existing git credential validation error shape so UI handling + does not need a new API family. +5. Keep runtime validation in `gitAdapter.Prepare` lightweight; do not spawn + `ssh-keygen` on every command. + +## Success Criteria + +- [ ] Save-time SSH validation test proves OpenSSH incompatibility is surfaced + before encryption/persistence. +- [ ] Valid unencrypted private keys still pass. +- [ ] Passphrase-protected keys remain rejected. +- [ ] No key material is logged or returned in validation errors. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-05-docs-ship.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-05-docs-ship.md new file mode 100644 index 00000000..17392453 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/phase-05-docs-ship.md @@ -0,0 +1,33 @@ +--- +phase: 5 +title: docs-ship +status: completed +effort: S +--- + +# Phase 5: docs-ship + +## Overview + +Update docs and run the requested git/ship flow. + +## Implementation Steps + +1. Update `docs/git-credential-adapter.md` and `docs/project-changelog.md` + with the Agent Access flow and PAT/SSH validation fixes. +2. Run focused tests: + - `go test ./internal/tools ./internal/http` + - `cd ui/web && pnpm test -- cli-credentials` +3. Run compile checks appropriate for touched code: + - `go build ./...` + - `go build -tags sqliteonly ./...` + - `cd ui/web && pnpm build` +4. Stage, secret-scan, commit, push, open PR to `dev`, wait for checks, reply to + issue #117 with evidence, and merge if checks pass. + +## Success Criteria + +- [ ] Plan validates with `ck plan validate --strict`. +- [ ] Red-team report has no unresolved blocker. +- [ ] Focused tests and compile checks pass or any external blocker is recorded. +- [ ] PR links issue #117 and includes validation evidence. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/plan.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/plan.md new file mode 100644 index 00000000..e52528d2 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/plan.md @@ -0,0 +1,81 @@ +--- +title: Issue 117 Agent Access Git Credential Follow-up +description: >- + TDD follow-up for issue #117 after PR #120: collapse the confusing Agent + Grants / Agent Credentials UI flow and fix git PAT / SSH credential validation + so injected credentials work in real agent git commands. +status: completed +priority: P1 +issue: 117 +branch: codex/issue-117-agent-credentials-git-followup +tags: [] +blockedBy: [] +blocks: [] +created: '2026-05-31T12:10:29.224Z' +createdBy: 'ck:plan' +source: skill +--- + +# Issue 117 Agent Access Git Credential Follow-up + +## Overview + +PR #120 added agent-scoped CLI credentials, but the follow-up evidence shows +two remaining defects: + +- Web UI can mount Agent Grants and Agent Credentials independently, so admins + can end up with overlapping modals for one binary. +- Production traces show agent credentials are selected and injected, yet git + still fails: HTTPS clone ignores the PAT header shape, and SSH accepts a key + at save time that OpenSSH later reports as `error in libcrypto`. + +Approach: + +1. Add failing characterization tests first for UI exclusivity and adapter + behavior. +2. Replace the two competing agent actions with one Agent Access flow. Keep + policy grants and typed secrets separate internally, but expose them as two + views of the same operator task. +3. Change GitHub PAT injection to Basic auth extraheader and scrub the encoded + secret form. +4. Validate SSH private keys against the OpenSSH parser before storing them, not + only against Go's `x/crypto/ssh` parser. +5. Validate with focused Go and Web UI tests before git/ship. + +## Phases + +| Phase | Name | Status | +|-------|------|--------| +| 1 | [characterization-tests](./phase-01-characterization-tests.md) | Completed | +| 2 | [agent-access-ui](./phase-02-agent-access-ui.md) | Completed | +| 3 | [git-pat-auth](./phase-03-git-pat-auth.md) | Completed | +| 4 | [ssh-key-validation](./phase-04-ssh-key-validation.md) | Completed | +| 5 | [docs-ship](./phase-05-docs-ship.md) | Completed | + +## Dependencies + +- Prior completed plan: `plans/260531-1545-agent-scoped-git-credentials/`. +- Related issue: `digitopvn/goclaw#117`. +- Related merged PRs: `digitopvn/goclaw#120`, `digitopvn/goclaw#96`. +- Runtime evidence: production logs show `credential_source=agent` for both PAT + and SSH paths, so this is an adapter/validation problem, not a missing + credential lookup problem. + +## Verified Facts + +- `CliCredentialsPanel` currently tracks `agentCredsTarget` and `grantsTarget` + as separate states and renders both dialogs if both are non-null. +- `gitAdapter.Prepare` currently writes + `GIT_CONFIG_VALUE_0=Authorization: Bearer ` for PAT credentials. +- `docs/git-credential-adapter.md` already documents GitHub PAT as Basic auth, + so code and docs diverged after PR #120. +- `prepareTypedCredentialEnv` validates SSH keys with `tools.ValidateSSHKey` + before encryption, but that only exercises Go's parser. + +## Out of Scope + +- GitHub App installation tokens or OAuth device-code minting. +- Wildcard host scopes. +- Passphrase-protected SSH keys. +- Changing credential precedence. Existing precedence remains user > context > + agent > binary env. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/redteam-report.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/redteam-report.md new file mode 100644 index 00000000..603b2872 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/redteam-report.md @@ -0,0 +1,30 @@ +# Red-team Report + +## Result + +Approved for implementation with two guardrails. + +## Findings + +1. Basic auth creates a second secret representation. Fix must scrub the raw PAT, + the base64 payload, and the full header value. Otherwise logs can hide + `ghp_...` while leaking `eC1hY2Nlc3MtdG9rZW46...`. +2. Do not run `ssh-keygen` on every agent git command. Runtime command execution + is hot path and already has key materialization. OpenSSH compatibility belongs + at save time. +3. Avoid nested Radix dialogs. The Agent Access implementation must extract + content bodies or keep one dialog root; rendering old dialog components inside + a new wrapper would keep the overlap class of bug. +4. Keep credential precedence unchanged. The production evidence shows agent + credentials are selected; changing resolver precedence would be a regression. + +## Decisions + +- Plan already includes scrub expansion in phase 3. +- Plan already constrains OpenSSH validation to storage path in phase 4. +- Plan phase 2 explicitly requires content extraction instead of nested dialogs. +- No blocker remains. + +## Unresolved Questions + +None. diff --git a/plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/validation-report.md b/plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/validation-report.md new file mode 100644 index 00000000..64013586 --- /dev/null +++ b/plans/260531-1910-issue-117-agent-access-git-credential-followup/reports/validation-report.md @@ -0,0 +1,18 @@ +# Validation Report + +## Result + +`ck plan validate plans/260531-1910-issue-117-agent-access-git-credential-followup/plan.md --strict` + +Passed: 0 errors, 0 warnings, 5 phases detected. + +## Claim Checks + +- UI overlap premise verified at `ui/web/src/pages/cli-credentials/cli-credentials-panel.tsx:40-41` and `:138-154`: agent credentials and grants are independent targets and can render independent dialog roots. +- PAT adapter mismatch verified at `internal/tools/credential_adapter_git.go:117-130`: current injected header is Bearer and only the raw token is scrubbed. +- SSH save-time parser gap verified at `internal/http/secure_cli_typed_credentials.go:129`: storage path calls only `tools.ValidateSSHKey`. +- Prior issue #117 plan is completed at `plans/260531-1545-agent-scoped-git-credentials/plan.md`, so this follow-up does not reopen the original storage/API scope. + +## Unresolved Questions + +None. diff --git a/tests/integration/git_adapter_pat_test.go b/tests/integration/git_adapter_pat_test.go index 92fc3a98..9d80ace9 100644 --- a/tests/integration/git_adapter_pat_test.go +++ b/tests/integration/git_adapter_pat_test.go @@ -6,7 +6,7 @@ package integration // // What we prove here that unit tests cannot: // - A real `git clone` over HTTP succeeds when the adapter's Injection env -// is passed through, against a server that requires the bearer token. +// is passed through, against a server that requires Basic PAT auth. // - The token never lands in `.git/config`, the remote URL, or any // credential helper after clone — i.e. the GIT_CONFIG_COUNT shape leaves // no on-disk trace. diff --git a/tests/integration/githttp_helper_test.go b/tests/integration/githttp_helper_test.go index 0a74378c..4acd1978 100644 --- a/tests/integration/githttp_helper_test.go +++ b/tests/integration/githttp_helper_test.go @@ -13,7 +13,7 @@ package integration // is self-contained — no network, no docker. // // Auth model: -// - Server checks `Authorization: Bearer ` on every request. +// - Server checks GitHub-style Basic auth on every request. // - Missing/wrong → 401, which makes git fail immediately. // - Matching → defer to git-http-backend CGI. // @@ -21,6 +21,7 @@ package integration // and skip if not present (some minimal CI images strip it). import ( + "encoding/base64" "net/http" "net/http/cgi" "net/http/httptest" @@ -32,7 +33,7 @@ import ( ) // startGitHTTPServer creates an httptest server backed by git-http-backend -// serving repos under projectRoot. Requests must carry the bearer token. +// serving repos under projectRoot. Requests must carry the Basic auth PAT. func startGitHTTPServer(t *testing.T, projectRoot, expectedToken string) *httptest.Server { t.Helper() @@ -58,7 +59,8 @@ func startGitHTTPServer(t *testing.T, projectRoot, expectedToken string) *httpte mux := http.NewServeMux() mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { got := r.Header.Get("Authorization") - if got != "Bearer "+expectedToken { + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("x-access-token:"+expectedToken)) + if got != want { http.Error(w, "unauthorized", http.StatusUnauthorized) return } diff --git a/ui/web/src/i18n/locales/en/cli-credentials.json b/ui/web/src/i18n/locales/en/cli-credentials.json index 77f53bcb..56198eba 100644 --- a/ui/web/src/i18n/locales/en/cli-credentials.json +++ b/ui/web/src/i18n/locales/en/cli-credentials.json @@ -70,6 +70,12 @@ "deleted": "CLI credential deleted", "deleteFailed": "Failed to delete credential" }, + "agentAccess": { + "title": "Agent Access", + "description": "Manage which agents can use {{name}} and which credential they use.", + "credentialsTab": "Credential", + "grantsTab": "Access policy" + }, "agentCredentials": { "title": "Agent Credentials", "description": "Configure PAT, SSH key, or env credentials that {{name}} can use when a selected agent runs it.", diff --git a/ui/web/src/i18n/locales/vi/cli-credentials.json b/ui/web/src/i18n/locales/vi/cli-credentials.json index 76f65515..ae5213de 100644 --- a/ui/web/src/i18n/locales/vi/cli-credentials.json +++ b/ui/web/src/i18n/locales/vi/cli-credentials.json @@ -70,6 +70,12 @@ "deleted": "Đã xóa thông tin CLI", "deleteFailed": "Không thể xóa thông tin" }, + "agentAccess": { + "title": "Truy cập Agent", + "description": "Quản lý agent nào được dùng {{name}} và credential mà agent đó sử dụng.", + "credentialsTab": "Credential", + "grantsTab": "Chính sách truy cập" + }, "agentCredentials": { "title": "Credential theo agent", "description": "Cấu hình PAT, khóa SSH hoặc biến môi trường để {{name}} dùng khi agent được chọn chạy lệnh.", diff --git a/ui/web/src/i18n/locales/zh/cli-credentials.json b/ui/web/src/i18n/locales/zh/cli-credentials.json index fee0f689..84887790 100644 --- a/ui/web/src/i18n/locales/zh/cli-credentials.json +++ b/ui/web/src/i18n/locales/zh/cli-credentials.json @@ -70,6 +70,12 @@ "deleted": "CLI 凭证已删除", "deleteFailed": "删除凭证失败" }, + "agentAccess": { + "title": "代理访问", + "description": "管理哪些代理可使用 {{name}} 以及该代理使用的凭证。", + "credentialsTab": "凭证", + "grantsTab": "访问策略" + }, "agentCredentials": { "title": "代理凭证", "description": "配置所选代理运行 {{name}} 时使用的 PAT、SSH 密钥或环境变量凭证。", diff --git a/ui/web/src/pages/cli-credentials/__tests__/cli-agent-credentials-wiring.test.ts b/ui/web/src/pages/cli-credentials/__tests__/cli-agent-credentials-wiring.test.ts index 00301b9d..7a1eea26 100644 --- a/ui/web/src/pages/cli-credentials/__tests__/cli-agent-credentials-wiring.test.ts +++ b/ui/web/src/pages/cli-credentials/__tests__/cli-agent-credentials-wiring.test.ts @@ -7,19 +7,22 @@ function source(path: string): string { } describe("CLI agent credential UI wiring", () => { - it("exposes Agent Credentials as a distinct table action before advanced user overrides", () => { + it("exposes one Agent Access action before advanced user overrides", () => { const table = source("src/pages/cli-credentials/cli-credentials-table.tsx"); - expect(table).toContain("onAgentCreds"); - expect(table).toContain("agentCredentials.title"); - expect(table.indexOf("onAgentCreds(item)")).toBeLessThan(table.indexOf("onUserCreds(item)")); + expect(table).toContain("onAgentAccess"); + expect(table).toContain("agentAccess.title"); + expect(table).not.toContain("onAgentCreds"); + expect(table.indexOf("onAgentAccess(item")).toBeLessThan(table.indexOf("onUserCreds(item)")); }); - it("mounts the Agent Credentials dialog from the panel", () => { + it("mounts a single Agent Access dialog from the panel", () => { const panel = source("src/pages/cli-credentials/cli-credentials-panel.tsx"); - expect(panel).toContain("cli-agent-credentials-dialog"); - expect(panel).toContain("CLIAgentCredentialsDialog"); - expect(panel).toContain("agentCredsTarget"); + expect(panel).toContain("cli-agent-access-dialog"); + expect(panel).toContain("CLIAgentAccessDialog"); + expect(panel).toContain("agentAccessTarget"); + expect(panel).not.toContain("agentCredsTarget"); + expect(panel).not.toContain("grantsTarget"); }); }); diff --git a/ui/web/src/pages/cli-credentials/cli-agent-access-dialog.tsx b/ui/web/src/pages/cli-credentials/cli-agent-access-dialog.tsx new file mode 100644 index 00000000..e2552921 --- /dev/null +++ b/ui/web/src/pages/cli-credentials/cli-agent-access-dialog.tsx @@ -0,0 +1,69 @@ +import { useEffect, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { Bot, ShieldCheck } from "lucide-react"; +import { Button } from "@/components/ui/button"; +import { + Dialog, DialogContent, DialogDescription, DialogHeader, DialogTitle, +} from "@/components/ui/dialog"; +import { CLIAgentCredentialsContent } from "./cli-agent-credentials-content"; +import { CliCredentialGrantsContent } from "./cli-credential-grants-content"; +import type { SecureCLIBinary } from "./hooks/use-cli-credentials"; + +export type AgentAccessTab = "credentials" | "grants"; + +interface Props { + open: boolean; + onOpenChange: (open: boolean) => void; + binary: SecureCLIBinary; + initialTab?: AgentAccessTab; +} + +export function CLIAgentAccessDialog({ open, onOpenChange, binary, initialTab = "credentials" }: Props) { + const { t } = useTranslation("cli-credentials"); + const [tab, setTab] = useState(initialTab); + + useEffect(() => { + if (open) setTab(initialTab); + }, [open, initialTab, binary.id]); + + return ( + + + + + + {t("agentAccess.title")} + + {t("agentAccess.description", { name: binary.binary_name })} + + +
+ + +
+ + {tab === "credentials" ? ( + onOpenChange(false)} /> + ) : ( + + )} + + + ); +} diff --git a/ui/web/src/pages/cli-credentials/cli-agent-credentials-content.tsx b/ui/web/src/pages/cli-credentials/cli-agent-credentials-content.tsx new file mode 100644 index 00000000..c6c55ca2 --- /dev/null +++ b/ui/web/src/pages/cli-credentials/cli-agent-credentials-content.tsx @@ -0,0 +1,193 @@ +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { Loader2, Plus } from "lucide-react"; +import { Button } from "@/components/ui/button"; +import { DialogFooter } from "@/components/ui/dialog"; +import { useAgents } from "@/pages/agents/hooks/use-agents"; +import { toast } from "@/stores/use-toast-store"; +import { type ManualEnvEntry } from "./cli-credential-env-vars-section"; +import { CliAgentCredentialForm } from "./cli-agent-credential-form"; +import { CliAgentCredentialList } from "./cli-agent-credential-list"; +import { + buildAgentCredentialPayload, + entriesFromEnv, +} from "./cli-agent-credentials-dialog-helpers"; +import { type GitCredentialType } from "./cli-credential-git-fields"; +import { useCliAgentCredentials, type CLIAgentCredential } from "./hooks/use-cli-agent-credentials"; +import type { SecureCLIBinary } from "./hooks/use-cli-credentials"; + +interface Props { + binary: SecureCLIBinary; + onClose: () => void; +} + +type ViewState = "list" | "form"; + +export function CLIAgentCredentialsContent({ binary, onClose }: Props) { + const { t } = useTranslation("cli-credentials"); + const { t: tc } = useTranslation("common"); + const { agents } = useAgents(); + const { agentCredentials, loading, getCredential, setCredential, deleteCredential } = useCliAgentCredentials(binary.id); + const isGit = binary.adapter_name === "git"; + + const [view, setView] = useState("list"); + const [editEntry, setEditEntry] = useState(null); + const [agentId, setAgentId] = useState(""); + const [envEntries, setEnvEntries] = useState([]); + const [saving, setSaving] = useState(false); + const [deleting, setDeleting] = useState(null); + const [gitType, setGitType] = useState("pat"); + const [gitHostScope, setGitHostScope] = useState("github.com"); + const [gitToken, setGitToken] = useState(""); + const [gitPrivateKey, setGitPrivateKey] = useState(""); + const [gitErrorKey, setGitErrorKey] = useState(); + const [gitHasExistingSecret, setGitHasExistingSecret] = useState(false); + + const agentNameMap = useMemo(() => { + const map = new Map(); + for (const a of agents) map.set(a.id, a.display_name || a.agent_key); + return map; + }, [agents]); + + const clearForm = () => { + setEditEntry(null); + setAgentId(""); + setEnvEntries([]); + setGitType(isGit ? "pat" : "env"); + setGitHostScope(isGit ? "github.com" : ""); + setGitToken(""); + setGitPrivateKey(""); + setGitErrorKey(undefined); + setGitHasExistingSecret(false); + }; + + useEffect(() => { + setView("list"); + clearForm(); + }, [binary.id]); + + const openAdd = () => { + clearForm(); + setView("form"); + }; + + const openEdit = async (entry: CLIAgentCredential) => { + setEditEntry(entry); + setAgentId(entry.agent_id); + setGitErrorKey(undefined); + setView("form"); + try { + const res = await getCredential(entry.agent_id); + setEnvEntries(entriesFromEnv(res.env)); + if (isGit) { + const nextType = (res.credential_type ?? "env") as GitCredentialType; + setGitType(nextType === "pat" || nextType === "ssh_key" ? nextType : "env"); + setGitHostScope(res.host_scope ?? "github.com"); + setGitHasExistingSecret(!!res.has_secret); + setGitToken(""); + setGitPrivateKey(""); + } + } catch { + setEnvEntries([]); + } + }; + + const handleSave = async () => { + if (!agentId) return; + setGitErrorKey(undefined); + const result = buildAgentCredentialPayload({ + isGit, + type: gitType, + hostScope: gitHostScope, + token: gitToken, + privateKey: gitPrivateKey, + hasExistingSecret: gitHasExistingSecret, + envEntries, + isNewEntry: editEntry === null, + }); + if (result.kind === "error") { + if (result.errorKey === "env_required") toast.error(t("agentCredentials.envRequired")); + else setGitErrorKey(result.errorKey); + return; + } + if (result.kind === "no_change") { + toast.success(t("agentCredentials.saved")); + setView("list"); + return; + } + setSaving(true); + try { + await setCredential(agentId, result.payload); + clearForm(); + setView("list"); + } catch (err) { + const code = (err as { code?: string })?.code; + if (code?.startsWith("git.cred_")) setGitErrorKey(code); + } finally { + setSaving(false); + } + }; + + const handleDelete = async (entry: CLIAgentCredential) => { + setDeleting(entry.agent_id); + try { + await deleteCredential(entry.agent_id); + } finally { + setDeleting(null); + } + }; + + return ( + <> +
+ {view === "list" ? ( + <> +

{t("agentCredentials.securityHint")}

+ {loading ?

{tc("loading")}

: null} + {agentCredentials.length === 0 ?

{t("agentCredentials.empty")}

: null} + + + ) : ( + + )} +
+ + {view === "list" ? ( + <> + + + + ) : ( + <> + + + + )} + + + ); +} diff --git a/ui/web/src/pages/cli-credentials/cli-agent-credentials-dialog.tsx b/ui/web/src/pages/cli-credentials/cli-agent-credentials-dialog.tsx index ddcc29ea..adfbc895 100644 --- a/ui/web/src/pages/cli-credentials/cli-agent-credentials-dialog.tsx +++ b/ui/web/src/pages/cli-credentials/cli-agent-credentials-dialog.tsx @@ -1,19 +1,9 @@ -import { useEffect, useMemo, useState } from "react"; import { useTranslation } from "react-i18next"; -import { Bot, Loader2, Plus } from "lucide-react"; -import { Button } from "@/components/ui/button"; -import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from "@/components/ui/dialog"; -import { useAgents } from "@/pages/agents/hooks/use-agents"; -import { toast } from "@/stores/use-toast-store"; -import { type ManualEnvEntry } from "./cli-credential-env-vars-section"; -import { CliAgentCredentialForm } from "./cli-agent-credential-form"; -import { CliAgentCredentialList } from "./cli-agent-credential-list"; +import { Bot } from "lucide-react"; import { - buildAgentCredentialPayload, - entriesFromEnv, -} from "./cli-agent-credentials-dialog-helpers"; -import { type GitCredentialType } from "./cli-credential-git-fields"; -import { useCliAgentCredentials, type CLIAgentCredential } from "./hooks/use-cli-agent-credentials"; + Dialog, DialogContent, DialogDescription, DialogHeader, DialogTitle, +} from "@/components/ui/dialog"; +import { CLIAgentCredentialsContent } from "./cli-agent-credentials-content"; import type { SecureCLIBinary } from "./hooks/use-cli-credentials"; interface Props { @@ -22,126 +12,8 @@ interface Props { binary: SecureCLIBinary; } -type ViewState = "list" | "form"; - export function CLIAgentCredentialsDialog({ open, onOpenChange, binary }: Props) { const { t } = useTranslation("cli-credentials"); - const { t: tc } = useTranslation("common"); - const { agents } = useAgents(); - const { agentCredentials, loading, getCredential, setCredential, deleteCredential } = useCliAgentCredentials(binary.id); - const isGit = binary.adapter_name === "git"; - - const [view, setView] = useState("list"); - const [editEntry, setEditEntry] = useState(null); - const [agentId, setAgentId] = useState(""); - const [envEntries, setEnvEntries] = useState([]); - const [saving, setSaving] = useState(false); - const [deleting, setDeleting] = useState(null); - const [gitType, setGitType] = useState("pat"); - const [gitHostScope, setGitHostScope] = useState("github.com"); - const [gitToken, setGitToken] = useState(""); - const [gitPrivateKey, setGitPrivateKey] = useState(""); - const [gitErrorKey, setGitErrorKey] = useState(); - const [gitHasExistingSecret, setGitHasExistingSecret] = useState(false); - - const agentNameMap = useMemo(() => { - const map = new Map(); - for (const a of agents) map.set(a.id, a.display_name || a.agent_key); - return map; - }, [agents]); - - const clearForm = () => { - setEditEntry(null); - setAgentId(""); - setEnvEntries([]); - setGitType(isGit ? "pat" : "env"); - setGitHostScope(isGit ? "github.com" : ""); - setGitToken(""); - setGitPrivateKey(""); - setGitErrorKey(undefined); - setGitHasExistingSecret(false); - }; - - useEffect(() => { - if (!open) { - clearForm(); - return; - } - setView("list"); - clearForm(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [open, binary.id]); - - const openAdd = () => { - clearForm(); - setView("form"); - }; - - const openEdit = async (entry: CLIAgentCredential) => { - setEditEntry(entry); - setAgentId(entry.agent_id); - setGitErrorKey(undefined); - setView("form"); - try { - const res = await getCredential(entry.agent_id); - setEnvEntries(entriesFromEnv(res.env)); - if (isGit) { - const nextType = (res.credential_type ?? "env") as GitCredentialType; - setGitType(nextType === "pat" || nextType === "ssh_key" ? nextType : "env"); - setGitHostScope(res.host_scope ?? "github.com"); - setGitHasExistingSecret(!!res.has_secret); - setGitToken(""); - setGitPrivateKey(""); - } - } catch { - setEnvEntries([]); - } - }; - - const handleSave = async () => { - if (!agentId) return; - setGitErrorKey(undefined); - const result = buildAgentCredentialPayload({ - isGit, - type: gitType, - hostScope: gitHostScope, - token: gitToken, - privateKey: gitPrivateKey, - hasExistingSecret: gitHasExistingSecret, - envEntries, - isNewEntry: editEntry === null, - }); - if (result.kind === "error") { - if (result.errorKey === "env_required") toast.error(t("agentCredentials.envRequired")); - else setGitErrorKey(result.errorKey); - return; - } - if (result.kind === "no_change") { - toast.success(t("agentCredentials.saved")); - setView("list"); - return; - } - setSaving(true); - try { - await setCredential(agentId, result.payload); - clearForm(); - setView("list"); - } catch (err) { - const code = (err as { code?: string })?.code; - if (code?.startsWith("git.cred_")) setGitErrorKey(code); - } finally { - setSaving(false); - } - }; - - const handleDelete = async (entry: CLIAgentCredential) => { - setDeleting(entry.agent_id); - try { - await deleteCredential(entry.agent_id); - } finally { - setDeleting(null); - } - }; return ( @@ -150,49 +22,7 @@ export function CLIAgentCredentialsDialog({ open, onOpenChange, binary }: Props) {t("agentCredentials.title")} {t("agentCredentials.description", { name: binary.binary_name })} -
- {view === "list" ? ( - <> -

{t("agentCredentials.securityHint")}

- {loading ?

{tc("loading")}

: null} - {agentCredentials.length === 0 ?

{t("agentCredentials.empty")}

: null} - - - ) : ( - - )} -
- - {view === "list" ? ( - <> - ) : ( - <> - )} - + onOpenChange(false)} /> ); diff --git a/ui/web/src/pages/cli-credentials/cli-credential-grants-content.tsx b/ui/web/src/pages/cli-credentials/cli-credential-grants-content.tsx new file mode 100644 index 00000000..46b24bcf --- /dev/null +++ b/ui/web/src/pages/cli-credentials/cli-credential-grants-content.tsx @@ -0,0 +1,151 @@ +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { Label } from "@/components/ui/label"; +import { useAgents } from "@/pages/agents/hooks/use-agents"; +import { useCliCredentialGrants } from "./hooks/use-cli-credentials"; +import { CliCredentialGrantCard } from "./cli-credential-grant-card"; +import { CliCredentialGrantForm } from "./cli-credential-grant-form"; +import { + EMPTY_ENV_STATE, buildEnvVarsPayload, envStateFromGrant, +} from "./cli-credential-grants-dialog-helpers"; +import type { GrantEnvState } from "./cli-credential-grant-env-section"; +import type { SecureCLIBinary, CLIAgentGrant } from "./hooks/use-cli-credentials"; + +interface Props { + binary: SecureCLIBinary; +} + +export function CliCredentialGrantsContent({ binary }: Props) { + const { t } = useTranslation("cli-credentials"); + const { t: tc } = useTranslation("common"); + const { agents } = useAgents(); + const { grants, loading, createGrant, updateGrant, deleteGrant } = useCliCredentialGrants(binary.id); + + const [agentId, setAgentId] = useState(""); + const [denyArgs, setDenyArgs] = useState(""); + const [denyVerbose, setDenyVerbose] = useState(""); + const [timeout, setTimeout] = useState(""); + const [tips, setTips] = useState(""); + const [enabled, setEnabled] = useState(true); + const [editingGrant, setEditingGrant] = useState(null); + const [saving, setSaving] = useState(false); + const [error, setError] = useState(""); + const [rejectedKeys, setRejectedKeys] = useState([]); + const [envState, setEnvState] = useState(EMPTY_ENV_STATE); + const [originalEnvSet, setOriginalEnvSet] = useState(false); + + const agentNameMap = useMemo(() => { + const map = new Map(); + for (const a of agents) map.set(a.id, a.display_name || a.agent_key); + return map; + }, [agents]); + + const clearForm = () => { + setAgentId(""); setDenyArgs(""); setDenyVerbose(""); setTimeout(""); setTips(""); + setEnabled(true); setEditingGrant(null); setError(""); setRejectedKeys([]); + setEnvState(EMPTY_ENV_STATE); setOriginalEnvSet(false); + }; + + useEffect(() => { clearForm(); }, [binary.id]); + + const selectGrant = (grant: CLIAgentGrant) => { + setAgentId(grant.agent_id); + setDenyArgs(grant.deny_args?.join(", ") ?? ""); + setDenyVerbose(grant.deny_verbose?.join(", ") ?? ""); + setTimeout(grant.timeout_seconds != null ? String(grant.timeout_seconds) : ""); + setTips(grant.tips ?? ""); + setEnabled(grant.enabled); + setEditingGrant(grant); + setError(""); setRejectedKeys([]); + setOriginalEnvSet(grant.env_set === true); + setEnvState(envStateFromGrant(grant)); + }; + + const splitComma = (v: string): string[] | null => { + const items = v.split(",").map((s) => s.trim()).filter(Boolean); + return items.length > 0 ? items : null; + }; + + const handleSubmit = async () => { + if (!agentId) { setError(t("grants.agentRequired")); return; } + setSaving(true); setError(""); setRejectedKeys([]); + try { + const envVarsPayload = buildEnvVarsPayload(envState, originalEnvSet); + const input = { + agent_id: agentId, + deny_args: splitComma(denyArgs), + deny_verbose: splitComma(denyVerbose), + timeout_seconds: timeout ? parseInt(timeout, 10) : null, + tips: tips.trim() || null, + enabled, + ...(envVarsPayload !== undefined ? { env_vars: envVarsPayload } : {}), + }; + if (editingGrant) await updateGrant(editingGrant.id, input); + else await createGrant(input); + clearForm(); + } catch (err) { + const msg = err instanceof Error ? err.message : tc("error"); + const details = (err as { details?: { rejected_keys?: string[] } }).details; + if (details?.rejected_keys) setRejectedKeys(details.rejected_keys); + setError(msg); + } finally { + setSaving(false); + } + }; + + const handleRevoke = async (grant: CLIAgentGrant) => { + setSaving(true); + try { + await deleteGrant(grant.id); + if (editingGrant?.id === grant.id) clearForm(); + } catch (err) { + setError(err instanceof Error ? err.message : tc("error")); + } finally { + setSaving(false); + } + }; + + return ( +
+ {grants.length > 0 && ( +
+ +
+ {grants.map((grant) => ( + selectGrant(grant)} + onRevoke={() => handleRevoke(grant)} + /> + ))} +
+
+ )} + + {loading &&

{tc("loading")}

} + {error &&

{error}

} +
+ ); +} diff --git a/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog.tsx b/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog.tsx index d2007213..c8860f73 100644 --- a/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog.tsx +++ b/ui/web/src/pages/cli-credentials/cli-credential-grants-dialog.tsx @@ -1,18 +1,9 @@ -import { useState, useEffect, useMemo } from "react"; import { useTranslation } from "react-i18next"; import { Dialog, DialogContent, DialogHeader, DialogTitle, } from "@/components/ui/dialog"; -import { Label } from "@/components/ui/label"; -import { useAgents } from "@/pages/agents/hooks/use-agents"; -import { useCliCredentialGrants } from "./hooks/use-cli-credentials"; -import { CliCredentialGrantCard } from "./cli-credential-grant-card"; -import { CliCredentialGrantForm } from "./cli-credential-grant-form"; -import { - EMPTY_ENV_STATE, buildEnvVarsPayload, envStateFromGrant, -} from "./cli-credential-grants-dialog-helpers"; -import type { GrantEnvState } from "./cli-credential-grant-env-section"; -import type { SecureCLIBinary, CLIAgentGrant } from "./hooks/use-cli-credentials"; +import { CliCredentialGrantsContent } from "./cli-credential-grants-content"; +import type { SecureCLIBinary } from "./hooks/use-cli-credentials"; interface Props { open: boolean; @@ -23,102 +14,6 @@ interface Props { /** Dialog for managing per-agent grants on a CLI credential. */ export function CliCredentialGrantsDialog({ open, onOpenChange, binary }: Props) { const { t } = useTranslation("cli-credentials"); - const { t: tc } = useTranslation("common"); - const { agents } = useAgents(); - const { grants, loading, createGrant, updateGrant, deleteGrant } = useCliCredentialGrants(binary.id); - - const [agentId, setAgentId] = useState(""); - const [denyArgs, setDenyArgs] = useState(""); - const [denyVerbose, setDenyVerbose] = useState(""); - const [timeout, setTimeout] = useState(""); - const [tips, setTips] = useState(""); - const [enabled, setEnabled] = useState(true); - const [editingGrant, setEditingGrant] = useState(null); - const [saving, setSaving] = useState(false); - const [error, setError] = useState(""); - const [rejectedKeys, setRejectedKeys] = useState([]); - - // Env override — local state only; cleared on dialog close (no persistent cache) - const [envState, setEnvState] = useState(EMPTY_ENV_STATE); - const [originalEnvSet, setOriginalEnvSet] = useState(false); - - const agentNameMap = useMemo(() => { - const map = new Map(); - for (const a of agents) map.set(a.id, a.display_name || a.agent_key); - return map; - }, [agents]); - - // Finding #11: clear form on both open AND close. - // Original code only cleared on open (true→?), leaving plaintext env values - // in state when dialog closes. Now: open=true → clearForm (fresh start); - // open=false → clearForm (wipe any revealed plaintext before next session). - useEffect(() => { clearForm(); }, [open]); // eslint-disable-line react-hooks/exhaustive-deps - - const clearForm = () => { - setAgentId(""); setDenyArgs(""); setDenyVerbose(""); setTimeout(""); setTips(""); - setEnabled(true); setEditingGrant(null); setError(""); setRejectedKeys([]); - setEnvState(EMPTY_ENV_STATE); setOriginalEnvSet(false); - }; - - const selectGrant = (grant: CLIAgentGrant) => { - setAgentId(grant.agent_id); - setDenyArgs(grant.deny_args?.join(", ") ?? ""); - setDenyVerbose(grant.deny_verbose?.join(", ") ?? ""); - setTimeout(grant.timeout_seconds != null ? String(grant.timeout_seconds) : ""); - setTips(grant.tips ?? ""); - setEnabled(grant.enabled); - setEditingGrant(grant); - setError(""); setRejectedKeys([]); - setOriginalEnvSet(grant.env_set === true); - setEnvState(envStateFromGrant(grant)); - }; - - const splitComma = (v: string): string[] | null => { - const items = v.split(",").map((s) => s.trim()).filter(Boolean); - return items.length > 0 ? items : null; - }; - - const handleSubmit = async () => { - if (!agentId) { setError(t("grants.agentRequired")); return; } - setSaving(true); setError(""); setRejectedKeys([]); - try { - const envVarsPayload = buildEnvVarsPayload(envState, originalEnvSet); - const input = { - agent_id: agentId, - deny_args: splitComma(denyArgs), - deny_verbose: splitComma(denyVerbose), - timeout_seconds: timeout ? parseInt(timeout, 10) : null, - tips: tips.trim() || null, - enabled, - ...(envVarsPayload !== undefined ? { env_vars: envVarsPayload } : {}), - }; - if (editingGrant) { - await updateGrant(editingGrant.id, input); - } else { - await createGrant(input); - } - clearForm(); - } catch (err) { - const msg = err instanceof Error ? err.message : tc("error"); - const details = (err as { details?: { rejected_keys?: string[] } }).details; - if (details?.rejected_keys) setRejectedKeys(details.rejected_keys); - setError(msg); - } finally { - setSaving(false); - } - }; - - const handleRevoke = async (grant: CLIAgentGrant) => { - setSaving(true); - try { - await deleteGrant(grant.id); - if (editingGrant?.id === grant.id) clearForm(); - } catch (err) { - setError(err instanceof Error ? err.message : tc("error")); - } finally { - setSaving(false); - } - }; return ( @@ -126,47 +21,7 @@ export function CliCredentialGrantsDialog({ open, onOpenChange, binary }: Props) {t("grants.title", { name: binary.binary_name })} -
- {grants.length > 0 && ( -
- -
- {grants.map((grant) => ( - selectGrant(grant)} - onRevoke={() => handleRevoke(grant)} - /> - ))} -
-
- )} - - {loading &&

{tc("loading")}

} - {error &&

{error}

} -
+ ); diff --git a/ui/web/src/pages/cli-credentials/cli-credentials-panel.tsx b/ui/web/src/pages/cli-credentials/cli-credentials-panel.tsx index eca1ce07..f0bff8f5 100644 --- a/ui/web/src/pages/cli-credentials/cli-credentials-panel.tsx +++ b/ui/web/src/pages/cli-credentials/cli-credentials-panel.tsx @@ -14,9 +14,9 @@ import { ConfirmDialog } from "@/components/shared/confirm-dialog"; import { useMinLoading } from "@/hooks/use-min-loading"; import { useDeferredLoading } from "@/hooks/use-deferred-loading"; import { useCliCredentials, useCliCredentialPresets } from "./hooks/use-cli-credentials"; -import { CliCredentialGrantsDialog } from "./cli-credential-grants-dialog"; import { CliCredentialsTable } from "./cli-credentials-table"; import type { SecureCLIBinary, CLICredentialInput } from "./hooks/use-cli-credentials"; +import type { AgentAccessTab } from "./cli-agent-access-dialog"; const CliCredentialFormDialog = lazy(() => import("./cli-credential-form-dialog").then((m) => ({ default: m.CliCredentialFormDialog })) @@ -24,10 +24,15 @@ const CliCredentialFormDialog = lazy(() => const CLIUserCredentialsDialog = lazy(() => import("./cli-user-credentials-dialog").then((m) => ({ default: m.CLIUserCredentialsDialog })) ); -const CLIAgentCredentialsDialog = lazy(() => - import("./cli-agent-credentials-dialog").then((m) => ({ default: m.CLIAgentCredentialsDialog })) +const CLIAgentAccessDialog = lazy(() => + import("./cli-agent-access-dialog").then((m) => ({ default: m.CLIAgentAccessDialog })) ); +type AgentAccessTarget = { + binary: SecureCLIBinary; + initialTab: AgentAccessTab; +}; + export function CliCredentialsPanel() { const { t } = useTranslation("cli-credentials"); const { t: tc } = useTranslation("common"); @@ -37,8 +42,7 @@ export function CliCredentialsPanel() { const [deleteTarget, setDeleteTarget] = useState(null); const [deleteLoading, setDeleteLoading] = useState(false); const [userCredsTarget, setUserCredsTarget] = useState(null); - const [agentCredsTarget, setAgentCredsTarget] = useState(null); - const [grantsTarget, setGrantsTarget] = useState(null); + const [agentAccessTarget, setAgentAccessTarget] = useState(null); const { items, loading, refresh, createCredential, updateCredential, deleteCredential } = useCliCredentials(); @@ -65,6 +69,9 @@ export function CliCredentialsPanel() { const openCreate = () => { setEditItem(null); setFormOpen(true); }; const openEdit = (item: SecureCLIBinary) => { setEditItem(item); setFormOpen(true); }; + const openAgentAccess = (item: SecureCLIBinary, initialTab: AgentAccessTab = "credentials") => { + setAgentAccessTarget({ binary: item, initialTab }); + }; return (
@@ -92,8 +99,7 @@ export function CliCredentialsPanel() { onEdit={openEdit} onDelete={setDeleteTarget} onUserCreds={setUserCredsTarget} - onAgentCreds={setAgentCredsTarget} - onGrants={setGrantsTarget} + onAgentAccess={openAgentAccess} /> {/* Finding #12: surface LIMIT 20 truncation so admins know there are more entries. */} {items.length >= 20 && ( @@ -135,23 +141,16 @@ export function CliCredentialsPanel() { )} - {agentCredsTarget && ( + {agentAccessTarget && ( - !open && setAgentCredsTarget(null)} - binary={agentCredsTarget} + !open && setAgentAccessTarget(null)} + binary={agentAccessTarget.binary} + initialTab={agentAccessTarget.initialTab} /> )} - - {grantsTarget && ( - !open && setGrantsTarget(null)} - binary={grantsTarget} - /> - )}
); } diff --git a/ui/web/src/pages/cli-credentials/cli-credentials-table.tsx b/ui/web/src/pages/cli-credentials/cli-credentials-table.tsx index 526c32f5..18b9ee15 100644 --- a/ui/web/src/pages/cli-credentials/cli-credentials-table.tsx +++ b/ui/web/src/pages/cli-credentials/cli-credentials-table.tsx @@ -4,7 +4,7 @@ * Phase 8: each row has a chip sub-row from agent_grants_summary. */ import { useTranslation } from "react-i18next"; -import { Bot, KeyRound, Pencil, Trash2, Users, Shield } from "lucide-react"; +import { KeyRound, Pencil, Trash2, Users, ShieldCheck } from "lucide-react"; import { Button } from "@/components/ui/button"; import { Badge } from "@/components/ui/badge"; import { CliCredentialAgentChips } from "./cli-credential-agent-chips"; @@ -15,11 +15,10 @@ interface Props { onEdit: (item: SecureCLIBinary) => void; onDelete: (item: SecureCLIBinary) => void; onUserCreds: (item: SecureCLIBinary) => void; - onAgentCreds: (item: SecureCLIBinary) => void; - onGrants: (item: SecureCLIBinary) => void; + onAgentAccess: (item: SecureCLIBinary, initialTab?: "credentials" | "grants") => void; } -export function CliCredentialsTable({ items, onEdit, onDelete, onUserCreds, onAgentCreds, onGrants }: Props) { +export function CliCredentialsTable({ items, onEdit, onDelete, onUserCreds, onAgentAccess }: Props) { const { t } = useTranslation("cli-credentials"); const { t: tc } = useTranslation("common"); @@ -71,15 +70,12 @@ export function CliCredentialsTable({ items, onEdit, onDelete, onUserCreds, onAg -