mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 08:18:19 +00:00
fix(tools): fail closed on github cli credentials (#154)
This commit is contained in:
1 parent
f077d56ea7
commit
9203782ae3
7 files changed
+160
-10
No files matched your search
@@ -110,10 +110,10 @@ Stored `gitea.example.com:8443` matches:
|
||||
If you run a self-hosted server on the scheme's default port (443 HTTPS, 22
|
||||
SSH), omit the port. If you run on a non-default port, include it.
|
||||
|
||||
When no stored credential matches the resolved remote host, the adapter falls
|
||||
through to the un-credentialed path: `git` runs with whatever credentials the
|
||||
calling shell already has (typically none, in which case the remote will reject
|
||||
the operation with a 401/403).
|
||||
When no typed PAT/SSH credential is selected, or the selected credential cannot
|
||||
match the resolved remote host, adapter-managed remote commands fail closed
|
||||
with a GoClaw diagnostic. `git` is not allowed to fall through to an
|
||||
interactive username/password prompt in agent runtime.
|
||||
|
||||
## Security model
|
||||
|
||||
|
||||
@@ -0,0 +1,31 @@
|
||||
# Secure CLI GitHub Credential Runtime Fix
|
||||
|
||||
## Summary
|
||||
|
||||
Fixed GitHub-related SecureCLI auth diagnostics for `git` and `gh` runtime
|
||||
commands linked to digitopvn/goclaw#138 and digitopvn/goclaw#151.
|
||||
|
||||
## Root Cause
|
||||
|
||||
- Adapter-managed `git` remote commands could reach raw Git when no typed PAT
|
||||
or SSH credential was selected.
|
||||
- Required env validation only covered `rapidapi`, so credentialed `gh`
|
||||
commands without `GH_TOKEN` could reach raw GitHub CLI auth guidance.
|
||||
|
||||
## Changes
|
||||
|
||||
- Added fail-closed git credential readiness validation before adapter prepare.
|
||||
- Generalized required env checks to preset non-optional env vars.
|
||||
- Added regression tests for missing git typed credentials and missing
|
||||
`GH_TOKEN`.
|
||||
- Updated git adapter docs and project changelog.
|
||||
|
||||
## Validation
|
||||
|
||||
- `/usr/local/go/bin/go test ./internal/tools`
|
||||
- `/usr/local/go/bin/go build ./...`
|
||||
- `/usr/local/go/bin/go build -tags sqliteonly ./...`
|
||||
|
||||
## Unresolved Questions
|
||||
|
||||
None.
|
||||
@@ -4,6 +4,25 @@ Significant changes, features, and fixes in reverse chronological order.
|
||||
|
||||
---
|
||||
|
||||
## 2026-06-11
|
||||
|
||||
### Secure CLI GitHub credential runtime diagnostics (issues #138, #151)
|
||||
|
||||
**Fixes**
|
||||
|
||||
- Git remote commands now fail closed before raw `git` auth prompts when no
|
||||
typed host-scoped PAT/SSH credential is selected.
|
||||
- Required SecureCLI env validation now applies to preset env vars such as
|
||||
`GH_TOKEN`, so credentialed `gh` commands report missing credential config
|
||||
instead of raw `gh auth login` guidance.
|
||||
|
||||
**Tests**
|
||||
|
||||
- Added regressions for `git push` without typed credentials and `gh` without
|
||||
`GH_TOKEN`.
|
||||
|
||||
---
|
||||
|
||||
## 2026-06-09
|
||||
|
||||
### Behavior UX sidecar delivery overrides (issue #144)
|
||||
|
||||
@@ -173,9 +173,6 @@ func ListPresetNames() []string {
|
||||
|
||||
func requiredCredentialEnvVars(binary string) []string {
|
||||
name := normalizeBinaryName(binary)
|
||||
if name != "rapidapi" {
|
||||
return nil
|
||||
}
|
||||
preset := GetPreset(name)
|
||||
if preset == nil {
|
||||
return nil
|
||||
|
||||
@@ -45,11 +45,11 @@ func TestRapidAPIPresetBlocksVerboseSecretLeakFlags(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequiredCredentialEnvVarsScopedToRapidAPI(t *testing.T) {
|
||||
func TestRequiredCredentialEnvVarsFromPresets(t *testing.T) {
|
||||
if got := requiredCredentialEnvVars("rapidapi"); !slices.Contains(got, "RAPIDAPI_KEY") {
|
||||
t.Fatalf("requiredCredentialEnvVars(rapidapi) = %v, missing RAPIDAPI_KEY", got)
|
||||
}
|
||||
if got := requiredCredentialEnvVars("gh"); len(got) != 0 {
|
||||
t.Fatalf("requiredCredentialEnvVars(gh) = %v, want no behavior change for existing presets", got)
|
||||
if got := requiredCredentialEnvVars("gh"); !slices.Contains(got, "GH_TOKEN") {
|
||||
t.Fatalf("requiredCredentialEnvVars(gh) = %v, missing GH_TOKEN", got)
|
||||
}
|
||||
}
|
||||
@@ -465,6 +465,9 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL
|
||||
|
||||
if adapter.ShouldInject(args) {
|
||||
userCred := userCredFromBinary(ctx, cred)
|
||||
if r := validateAdapterCredentialReady(adapter.Name(), binary, userCred); r != nil {
|
||||
return r
|
||||
}
|
||||
// Plant the resolved exec cwd so adapters (e.g. git) can run any
|
||||
// pre-flight sub-exec from the right repo, not goclaw's daemon CWD.
|
||||
prepareCtx := WithExecCwd(ctx, cwd)
|
||||
@@ -542,6 +545,26 @@ func credentialSubjectForAdapter(ctx context.Context, bin *store.SecureCLIBinary
|
||||
return store.CredentialUserIDFromContext(ctx)
|
||||
}
|
||||
|
||||
func validateAdapterCredentialReady(adapterName, binary string, cred *store.SecureCLIUserCredential) *Result {
|
||||
if adapterName != "git" {
|
||||
return nil
|
||||
}
|
||||
if cred == nil || cred.CredentialType == nil || strings.TrimSpace(*cred.CredentialType) == "" {
|
||||
return credentialedGitCredentialResolutionError(binary, "no typed git credential selected")
|
||||
}
|
||||
switch strings.TrimSpace(*cred.CredentialType) {
|
||||
case "pat", "ssh_key":
|
||||
if cred.HostScope == nil || strings.TrimSpace(*cred.HostScope) == "" {
|
||||
return credentialedGitCredentialResolutionError(binary, "typed git credential is missing host_scope")
|
||||
}
|
||||
return nil
|
||||
case "env":
|
||||
return credentialedGitCredentialResolutionError(binary, "legacy env credential cannot authenticate adapter-managed git remotes")
|
||||
default:
|
||||
return nil
|
||||
}
|
||||
}
|
||||
|
||||
func effectiveHostScope(bin *store.SecureCLIBinary) *string {
|
||||
if bin == nil {
|
||||
return nil
|
||||
@@ -908,6 +931,17 @@ func credentialedMissingEnvError(binary string, keys []string) *Result {
|
||||
}
|
||||
}
|
||||
|
||||
func credentialedGitCredentialResolutionError(binary, reason string) *Result {
|
||||
return &Result{
|
||||
ForLLM: fmt.Sprintf("[CREDENTIALED EXEC] Git credential resolution failed.\n"+
|
||||
"Binary: %s\nReason: %s\n"+
|
||||
"Configure a SecureCLI git credential with Credential Type = Personal Access Token or SSH Private Key and Host Scope matching the remote host, then grant this binary to the target agent.",
|
||||
binary, reason),
|
||||
ForUser: "Git credential resolution failed. Configure a host-scoped PAT or SSH key for this agent.",
|
||||
IsError: true,
|
||||
}
|
||||
}
|
||||
|
||||
func credentialedExecFailError(binary string, args []string, exitCode int, output string) *Result {
|
||||
return &Result{
|
||||
ForLLM: fmt.Sprintf("[CREDENTIALED EXEC] Command failed (exit code %d).\n"+
|
||||
|
||||
@@ -183,3 +183,72 @@ func TestExec_RapidAPIWithRequiredEnvReachesDirectExec(t *testing.T) {
|
||||
t.Fatalf("credential value leaked into output: %s", result.ForLLM)
|
||||
}
|
||||
}
|
||||
|
||||
func TestExec_GHMissingRequiredEnvFailsBeforeRawAuth(t *testing.T) {
|
||||
stub := newStubSecureCLIStore()
|
||||
stub.byName["gh"] = &store.SecureCLIBinary{
|
||||
BinaryName: "gh",
|
||||
EncryptedEnv: []byte("{}"),
|
||||
TimeoutSeconds: 10,
|
||||
DenyArgs: json.RawMessage("[]"),
|
||||
DenyVerbose: json.RawMessage("[]"),
|
||||
}
|
||||
|
||||
tool := NewExecTool(t.TempDir(), false)
|
||||
tool.SetSecureCLIStore(stub)
|
||||
|
||||
ctx := store.WithTenantID(store.WithAgentID(context.Background(), uuid.New()), uuid.New())
|
||||
result := tool.Execute(ctx, map[string]any{"command": "gh issue list --repo digitopvn/goclaw --limit 1"})
|
||||
|
||||
if !result.IsError {
|
||||
t.Fatalf("expected missing GH_TOKEN to fail")
|
||||
}
|
||||
if !strings.Contains(result.ForLLM, "missing required credential env") {
|
||||
t.Fatalf("expected actionable missing-env error, got: %s", result.ForLLM)
|
||||
}
|
||||
if !strings.Contains(result.ForLLM, "GH_TOKEN") {
|
||||
t.Fatalf("expected GH_TOKEN in error, got: %s", result.ForLLM)
|
||||
}
|
||||
if strings.Contains(result.ForLLM, "gh auth login") {
|
||||
t.Fatalf("raw gh auth guidance leaked through: %s", result.ForLLM)
|
||||
}
|
||||
}
|
||||
|
||||
func TestExec_GitRemoteCommandWithoutTypedCredentialFailsBeforeRawAuth(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("shell script fixture is POSIX-only")
|
||||
}
|
||||
|
||||
binDir := t.TempDir()
|
||||
binPath := filepath.Join(binDir, "git")
|
||||
if err := os.WriteFile(binPath, []byte("#!/bin/sh\nprintf 'raw git reached\\n' >&2\nexit 128\n"), 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
adapterName := "git"
|
||||
stub := newStubSecureCLIStore()
|
||||
stub.byName["git"] = &store.SecureCLIBinary{
|
||||
BinaryName: "git",
|
||||
BinaryPath: &binPath,
|
||||
EncryptedEnv: []byte("{}"),
|
||||
TimeoutSeconds: 10,
|
||||
DenyArgs: json.RawMessage("[]"),
|
||||
DenyVerbose: json.RawMessage("[]"),
|
||||
AdapterName: &adapterName,
|
||||
}
|
||||
|
||||
tool := NewExecTool(t.TempDir(), false)
|
||||
tool.SetSecureCLIStore(stub)
|
||||
|
||||
ctx := store.WithTenantID(store.WithAgentID(context.Background(), uuid.New()), uuid.New())
|
||||
result := tool.Execute(ctx, map[string]any{"command": "git push -u origin feature"})
|
||||
|
||||
if !result.IsError {
|
||||
t.Fatalf("expected missing typed git credential to fail")
|
||||
}
|
||||
if !strings.Contains(result.ForLLM, "Git credential resolution failed") {
|
||||
t.Fatalf("expected git credential diagnostic, got: %s", result.ForLLM)
|
||||
}
|
||||
if strings.Contains(result.ForLLM, "raw git reached") {
|
||||
t.Fatalf("raw git command executed before credential resolution failed: %s", result.ForLLM)
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user