mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-13 16:14:57 +00:00
fix(rapidapi): add cron credential diagnostics
Squash merge PR #101 after resolving dev conflicts and fixing cron GetJob snapshot race. PR CI run 26703877225 passed release-versioning, go, and web.
This commit is contained in:
1 parent
4ed4eb9509
commit
cf66bda618
12 files changed
+700
-14
No files matched your search
@@ -6,6 +6,22 @@ Significant changes, features, and fixes in reverse chronological order.
|
||||
|
||||
## 2026-05-29
|
||||
|
||||
### RapidAPI cron SecureCLI diagnostics (issue #74)
|
||||
|
||||
**Fixes**
|
||||
|
||||
- Added a built-in `rapidapi` SecureCLI preset with required `RAPIDAPI_KEY`, 60s timeout, and verbose/debug flag denials.
|
||||
- Credentialed exec now validates required preset env keys before binary resolution, so missing RapidAPI credentials return a GoClaw diagnostic instead of downstream `RAPIDAPI_KEY required`.
|
||||
- Added safe SecureCLI env diagnostics that log env key names/count context only, never credential values.
|
||||
- Added `RAPIDAPI_KEY` to fall-through exec env scrubbing.
|
||||
|
||||
**Tests**
|
||||
|
||||
- Added RapidAPI preset contract and deny-pattern coverage.
|
||||
- Added credentialed exec regressions for missing RapidAPI env and successful direct exec with injected `RAPIDAPI_KEY`.
|
||||
|
||||
---
|
||||
|
||||
### Sandbox tenant workspace isolation (issue #68)
|
||||
|
||||
**Security**
|
||||
|
||||
@@ -214,7 +214,8 @@ func (cs *Service) GetJob(jobID string) (*Job, bool) {
|
||||
|
||||
for i, job := range cs.store.Jobs {
|
||||
if job.ID == jobID {
|
||||
return &cs.store.Jobs[i], true
|
||||
jobCopy := cs.store.Jobs[i]
|
||||
return &jobCopy, true
|
||||
}
|
||||
}
|
||||
return nil, false
|
||||
|
||||
@@ -139,6 +139,17 @@ var CLIPresets = map[string]CLIPreset{
|
||||
Tips: "Use -A -t for plain output suitable for piping",
|
||||
AdapterName: "psql",
|
||||
},
|
||||
"rapidapi": {
|
||||
BinaryName: "rapidapi",
|
||||
Description: "RapidAPI CLI",
|
||||
EnvVars: []EnvVarDef{
|
||||
{Name: "RAPIDAPI_KEY", Desc: "RapidAPI application key"},
|
||||
},
|
||||
DenyArgs: nil,
|
||||
DenyVerbose: []string{`--verbose`, `--debug`, `-v`},
|
||||
Timeout: 60,
|
||||
Tips: "Use read-only/search commands for scheduled jobs. Configure RAPIDAPI_KEY as a per-user credential and grant the target agent.",
|
||||
},
|
||||
}
|
||||
|
||||
// GetPreset returns a preset by name, or nil if not found.
|
||||
@@ -159,3 +170,23 @@ func ListPresetNames() []string {
|
||||
sort.Strings(names)
|
||||
return names
|
||||
}
|
||||
|
||||
func requiredCredentialEnvVars(binary string) []string {
|
||||
name := normalizeBinaryName(binary)
|
||||
if name != "rapidapi" {
|
||||
return nil
|
||||
}
|
||||
preset := GetPreset(name)
|
||||
if preset == nil {
|
||||
return nil
|
||||
}
|
||||
names := make([]string, 0, len(preset.EnvVars))
|
||||
for _, envVar := range preset.EnvVars {
|
||||
if envVar.Optional {
|
||||
continue
|
||||
}
|
||||
names = append(names, envVar.Name)
|
||||
}
|
||||
sort.Strings(names)
|
||||
return names
|
||||
}
|
||||
@@ -0,0 +1,55 @@
|
||||
package tools
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"slices"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestRapidAPIPresetContract(t *testing.T) {
|
||||
preset := GetPreset("rapidapi")
|
||||
if preset == nil {
|
||||
t.Fatal("rapidapi preset is missing")
|
||||
}
|
||||
if preset.BinaryName != "rapidapi" {
|
||||
t.Fatalf("BinaryName = %q, want rapidapi", preset.BinaryName)
|
||||
}
|
||||
|
||||
envNames := make([]string, 0, len(preset.EnvVars))
|
||||
for _, envVar := range preset.EnvVars {
|
||||
envNames = append(envNames, envVar.Name)
|
||||
}
|
||||
if !slices.Contains(envNames, "RAPIDAPI_KEY") {
|
||||
t.Fatalf("rapidapi preset env vars = %v, missing RAPIDAPI_KEY", envNames)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRapidAPIPresetBlocksVerboseSecretLeakFlags(t *testing.T) {
|
||||
preset := GetPreset("rapidapi")
|
||||
if preset == nil {
|
||||
t.Fatal("rapidapi preset is missing")
|
||||
}
|
||||
denyVerbose, err := json.Marshal(preset.DenyVerbose)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
for _, args := range [][]string{
|
||||
{"--verbose"},
|
||||
{"--debug"},
|
||||
{"-v"},
|
||||
} {
|
||||
if got := matchesBinaryVerbose(args, denyVerbose); got == "" {
|
||||
t.Fatalf("matchesBinaryVerbose(%v) did not block verbose/debug flag", args)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequiredCredentialEnvVarsScopedToRapidAPI(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)
|
||||
}
|
||||
}
|
||||
@@ -374,17 +374,7 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL
|
||||
return credentialedShellOperatorError(rawCommand, ops)
|
||||
}
|
||||
|
||||
// Step 2: Resolve binary to absolute path and verify against config
|
||||
absPath, err := resolveAndMatchBinary(binary, cred.BinaryPath)
|
||||
if err != nil {
|
||||
r := credentialedPathError(binary, err)
|
||||
if t.sandboxMgr != nil && sandboxKey != "" {
|
||||
r.ForLLM += hintBinaryNotFound
|
||||
}
|
||||
return r
|
||||
}
|
||||
|
||||
// Step 3: Per-binary deny check (deny_args)
|
||||
// Step 2: Per-binary deny check (deny_args)
|
||||
denyArgs, allowAudits := applyCommandKeywordAllowlist(binary, args, t.commandKeywordAllowlistSnapshot())
|
||||
if p := matchesBinaryDeny(denyArgs, cred.DenyArgs); p != "" {
|
||||
return credentialedDenyError(binary, args, p)
|
||||
@@ -409,18 +399,46 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL
|
||||
)
|
||||
}
|
||||
|
||||
// Step 4: Decrypt env vars from store (already decrypted by store layer).
|
||||
// Step 3: Decrypt env vars from store (already decrypted by store layer).
|
||||
// Per-user env overrides take priority over binary/grant env.
|
||||
envMap, err := mergeCredentialedEnv(cred)
|
||||
if err != nil {
|
||||
return ErrorResult(fmt.Sprintf("credentialed exec: invalid env JSON for %q: %v", binary, err))
|
||||
}
|
||||
slog.Debug("secure_cli.env_merged",
|
||||
"binary", binary,
|
||||
"agent_id", store.AgentIDFromContext(ctx),
|
||||
"tenant_id", store.TenantIDFromContext(ctx),
|
||||
"credential_user_id_present", store.CredentialUserIDFromContext(ctx) != "",
|
||||
"env_keys", sortedKeys(envMap),
|
||||
)
|
||||
if missing := missingRequiredCredentialEnv(binary, envMap); len(missing) > 0 {
|
||||
slog.Warn("secure_cli.missing_required_env",
|
||||
"binary", binary,
|
||||
"agent_id", store.AgentIDFromContext(ctx),
|
||||
"tenant_id", store.TenantIDFromContext(ctx),
|
||||
"credential_user_id_present", store.CredentialUserIDFromContext(ctx) != "",
|
||||
"missing_env_keys", missing,
|
||||
"env_keys", sortedKeys(envMap),
|
||||
)
|
||||
return credentialedMissingEnvError(binary, missing)
|
||||
}
|
||||
|
||||
// Step 5: Register credential values for output scrubbing
|
||||
// Step 4: Register credential values for output scrubbing
|
||||
for _, v := range envMap {
|
||||
AddCredentialScrubValues(v)
|
||||
}
|
||||
|
||||
// Step 5: Resolve binary to absolute path and verify against config
|
||||
absPath, err := resolveAndMatchBinary(binary, cred.BinaryPath)
|
||||
if err != nil {
|
||||
r := credentialedPathError(binary, err)
|
||||
if t.sandboxMgr != nil && sandboxKey != "" {
|
||||
r.ForLLM += hintBinaryNotFound
|
||||
}
|
||||
return r
|
||||
}
|
||||
|
||||
// Step 6: Determine timeout
|
||||
timeout := time.Duration(cred.TimeoutSeconds) * time.Second
|
||||
if timeout <= 0 {
|
||||
@@ -531,6 +549,20 @@ func mergeCredentialedEnv(cred *store.SecureCLIBinary) (map[string]string, error
|
||||
return envMap, nil
|
||||
}
|
||||
|
||||
func missingRequiredCredentialEnv(binary string, envMap map[string]string) []string {
|
||||
required := requiredCredentialEnvVars(binary)
|
||||
if len(required) == 0 {
|
||||
return nil
|
||||
}
|
||||
missing := make([]string, 0, len(required))
|
||||
for _, key := range required {
|
||||
if strings.TrimSpace(envMap[key]) == "" {
|
||||
missing = append(missing, key)
|
||||
}
|
||||
}
|
||||
return missing
|
||||
}
|
||||
|
||||
// validateExecCwd checks that cmd.Dir exists and is a directory before
|
||||
// cmd.Start(). Empty cwd is allowed (Go interprets as "inherit parent's dir").
|
||||
//
|
||||
@@ -825,6 +857,17 @@ func credentialedDenyError(binary string, args []string, pattern string) *Result
|
||||
}
|
||||
}
|
||||
|
||||
func credentialedMissingEnvError(binary string, keys []string) *Result {
|
||||
return &Result{
|
||||
ForLLM: fmt.Sprintf("[CREDENTIALED EXEC] Credential config missing required credential env.\n"+
|
||||
"Binary: %s\nMissing env keys: %s\n"+
|
||||
"Configure the missing key as a SecureCLI user credential for the same user context, then grant this binary to the target agent.",
|
||||
binary, strings.Join(keys, ", ")),
|
||||
ForUser: fmt.Sprintf("CLI credential for %q is missing required env: %s.", binary, strings.Join(keys, ", ")),
|
||||
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"+
|
||||
|
||||
@@ -1,8 +1,16 @@
|
||||
package tools
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/store"
|
||||
)
|
||||
|
||||
@@ -64,3 +72,76 @@ func TestMergeCredentialedEnvFlattensSensitiveValueEntries(t *testing.T) {
|
||||
t.Fatalf("PUBLIC_BASE_URL = %q", env["PUBLIC_BASE_URL"])
|
||||
}
|
||||
}
|
||||
|
||||
func TestExec_RapidAPIMissingRequiredEnvFailsBeforeBinaryResolution(t *testing.T) {
|
||||
stub := newStubSecureCLIStore()
|
||||
stub.byName["rapidapi"] = &store.SecureCLIBinary{
|
||||
BinaryName: "rapidapi",
|
||||
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())
|
||||
ctx = store.WithCredentialUserID(ctx, "tenant-user-rapidapi")
|
||||
result := tool.Execute(ctx, map[string]any{"command": "rapidapi search weather"})
|
||||
|
||||
if !result.IsError {
|
||||
t.Fatalf("expected missing RAPIDAPI_KEY 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, "RAPIDAPI_KEY") {
|
||||
t.Fatalf("expected missing key name in error, got: %s", result.ForLLM)
|
||||
}
|
||||
if strings.Contains(result.ForLLM, "not found in PATH") || strings.Contains(result.ForLLM, "Binary resolution failed") {
|
||||
t.Fatalf("missing env should be reported before binary resolution, got: %s", result.ForLLM)
|
||||
}
|
||||
if strings.Contains(result.ForLLM, "tenant-user-rapidapi") {
|
||||
t.Fatalf("credential user id leaked into user-facing error: %s", result.ForLLM)
|
||||
}
|
||||
}
|
||||
|
||||
func TestExec_RapidAPIWithRequiredEnvReachesDirectExec(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("shell script fixture is POSIX-only")
|
||||
}
|
||||
|
||||
binDir := t.TempDir()
|
||||
binPath := filepath.Join(binDir, "rapidapi")
|
||||
if err := os.WriteFile(binPath, []byte("#!/bin/sh\n[ -n \"$RAPIDAPI_KEY\" ] || exit 43\nprintf 'rapidapi args:%s\\n' \"$*\"\n"), 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
stub := newStubSecureCLIStore()
|
||||
stub.byName["rapidapi"] = &store.SecureCLIBinary{
|
||||
BinaryName: "rapidapi",
|
||||
BinaryPath: &binPath,
|
||||
EncryptedEnv: []byte("{}"),
|
||||
UserEnv: []byte(`{"RAPIDAPI_KEY":"test-key-not-real"}`),
|
||||
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": "rapidapi search weather"})
|
||||
|
||||
if result.IsError {
|
||||
t.Fatalf("expected rapidapi direct exec to run, got: %s", result.ForLLM)
|
||||
}
|
||||
if !strings.Contains(result.ForLLM, "rapidapi args:search weather") {
|
||||
t.Fatalf("expected script output, got: %s", result.ForLLM)
|
||||
}
|
||||
if strings.Contains(result.ForLLM, "test-key-not-real") {
|
||||
t.Fatalf("credential value leaked into output: %s", result.ForLLM)
|
||||
}
|
||||
}
|
||||
@@ -24,6 +24,7 @@ var staticCredentialEnvKeys = []string{
|
||||
"AWS_SECRET_ACCESS_KEY",
|
||||
"AWS_SESSION_TOKEN",
|
||||
"GOOGLE_APPLICATION_CREDENTIALS",
|
||||
"RAPIDAPI_KEY",
|
||||
"HUGGINGFACE_TOKEN",
|
||||
"HF_TOKEN",
|
||||
"GCP_SA_KEY",
|
||||
|
||||
@@ -21,6 +21,7 @@ func TestScrubCredentialEnv_StripsStatic(t *testing.T) {
|
||||
"GH_TOKEN=secret-abc",
|
||||
"PATH=/usr/bin",
|
||||
"AWS_SECRET_ACCESS_KEY=topsecret",
|
||||
"RAPIDAPI_KEY=rapid-secret",
|
||||
}
|
||||
out := scrubCredentialEnv(in, nil)
|
||||
|
||||
@@ -30,6 +31,9 @@ func TestScrubCredentialEnv_StripsStatic(t *testing.T) {
|
||||
if envContains(out, "AWS_SECRET_ACCESS_KEY") {
|
||||
t.Fatalf("AWS_SECRET_ACCESS_KEY must be scrubbed, got: %v", out)
|
||||
}
|
||||
if envContains(out, "RAPIDAPI_KEY") {
|
||||
t.Fatalf("RAPIDAPI_KEY must be scrubbed, got: %v", out)
|
||||
}
|
||||
if !envContains(out, "HOME") || !envContains(out, "PATH") {
|
||||
t.Fatalf("essential vars must be preserved, got: %v", out)
|
||||
}
|
||||
|
||||
+124
@@ -0,0 +1,124 @@
|
||||
---
|
||||
phase: 1
|
||||
title: "Diagnose Current RapidAPI Credential Path"
|
||||
status: complete
|
||||
priority: P1
|
||||
effort: "1.5h"
|
||||
dependencies: []
|
||||
---
|
||||
|
||||
# Phase 1: Diagnose Current RapidAPI Credential Path
|
||||
|
||||
## Context Links
|
||||
|
||||
- Issue: `digitopvn/goclaw#74`
|
||||
- Related closed issue: `digitopvn/goclaw#54`
|
||||
- Cron payload model: `internal/store/cron_store.go`
|
||||
- Cron run injection: `cmd/gateway_cron.go`
|
||||
- SecureCLI lookup: `internal/tools/credentialed_exec.go`
|
||||
|
||||
## Overview
|
||||
|
||||
Prove the concrete failure mode before code. Current code already preserves cron credential user identity; #74 may be RapidAPI config, grant, env key, or legacy job payload.
|
||||
|
||||
## Key Insights
|
||||
|
||||
- `CronPayload.CredentialUserID` exists and is redacted from API responses.
|
||||
- `PGCronStore.AddJob` and `SQLiteCronStore.AddJob` store explicit credential user identity.
|
||||
- `makeCronJobHandler` injects that identity into cron run context.
|
||||
- `lookupCredentialedBinary` resolves per-user env through `CredentialUserIDFromContext`.
|
||||
- No built-in RapidAPI preset exists today.
|
||||
- 2026-05-29 verification:
|
||||
- #54 context path is present: cron handler injects `payload.credentialUserId` before scheduling.
|
||||
- Redaction path is present: response copies clear `payload.credentialUserId`.
|
||||
- `rapidapi` is absent from `CLIPresets`, so operators get no guided `RAPIDAPI_KEY` setup.
|
||||
- Credentialed exec merges base/user env but does not validate required preset env keys before process execution.
|
||||
|
||||
## Requirements
|
||||
|
||||
- Functional: identify whether `rapidapi` is configured, granted, and populated with `RAPIDAPI_KEY`.
|
||||
- Functional: identify if failing cron payload is legacy and lacks `credentialUserId`.
|
||||
- Non-functional: no secret values in terminal, docs, issue comment, tests, or logs.
|
||||
- Non-functional: read-only live checks unless operator explicitly approves mutation.
|
||||
|
||||
## Architecture
|
||||
|
||||
Cron creation captures credential identity in payload. Cron execution injects that identity into context. Exec tool uses context to join SecureCLI per-user env. Diagnosis must trace all three links.
|
||||
|
||||
```text
|
||||
cron.create ctx -> payload.credentialUserId -> makeCronJobHandler ctx -> exec LookupByBinary(userID) -> RAPIDAPI_KEY env
|
||||
```
|
||||
|
||||
## Related Code Files
|
||||
|
||||
- Modify: none in this phase.
|
||||
- Read: `internal/store/pg/cron_crud.go`
|
||||
- Read: `internal/store/sqlitestore/cron_crud.go`
|
||||
- Read: `cmd/gateway_cron.go`
|
||||
- Read: `internal/tools/credentialed_exec.go`
|
||||
- Read: `internal/store/pg/secure_cli.go`
|
||||
- Read: `internal/store/sqlitestore/secure-cli.go`
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
1. Write failing characterization notes before code:
|
||||
- `rapidapi` absent from SecureCLI registry.
|
||||
- `rapidapi` present but grant missing/disabled.
|
||||
- grant present but `RAPIDAPI_KEY` missing from merged env.
|
||||
- cron payload lacks `credentialUserId`.
|
||||
2. Inspect local DB or live DB only with read-only commands if credentials available:
|
||||
- SecureCLI binary rows for `rapidapi`.
|
||||
- grants for the cron agent.
|
||||
- sanitized env keys only; never reveal value.
|
||||
- cron payload JSON for target job.
|
||||
3. Add or prepare unit-level characterization tests before changing product code:
|
||||
- cron handler passes `credentialUserId` through.
|
||||
- SecureCLI env merge exposes `RAPIDAPI_KEY` when configured.
|
||||
- missing user env produces actionable branch in diagnostics plan.
|
||||
4. Decide root cause:
|
||||
- config-only: document operator fix and still consider preset/logging.
|
||||
- code gap: proceed Phase 2.
|
||||
|
||||
## Todo List
|
||||
|
||||
- [x] Confirm current branch contains #54 fix.
|
||||
- [x] Confirm `rapidapi` preset absence.
|
||||
- [x] Inspect sanitized SecureCLI config path.
|
||||
- [ ] Inspect failing cron payload shape if available.
|
||||
- [x] Record exact failure class.
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [x] Root cause class documented with code/file evidence.
|
||||
- [x] No secret material displayed or persisted.
|
||||
- [x] Tests-before list finalized for Phase 2.
|
||||
|
||||
## Diagnosis Result
|
||||
|
||||
Root cause class: product support gap plus weak diagnostics, not missing cron identity plumbing.
|
||||
|
||||
Code evidence:
|
||||
|
||||
- `cmd/gateway_cron.go` injects `store.WithCredentialUserID` when `job.Payload.CredentialUserID` is present.
|
||||
- `internal/store/cron_store.go` redacts `CredentialUserID` from response-safe cron jobs.
|
||||
- `internal/tools/credentialed_exec.go` uses `store.CredentialUserIDFromContext(ctx)` for `LookupByBinary`.
|
||||
- `internal/tools/credential_presets.go` has no `rapidapi` preset.
|
||||
- `internal/tools/credentialed_exec.go` returns downstream CLI output when env is empty, so a RapidAPI failure surfaces as raw `RAPIDAPI_KEY required` instead of a GoClaw credential diagnostic.
|
||||
|
||||
Live DB inspection was not run in this phase because no operator-approved credentialed environment or target cron job was provided. Phase 2 proceeds with code-level regression coverage for the confirmed local gap.
|
||||
|
||||
## Risk Assessment
|
||||
|
||||
- Risk: live DB access reveals secrets. Mitigation: query key names/sanitized responses only.
|
||||
- Risk: legacy cron payload mistaken for current bug. Mitigation: compare job creation time against fix version.
|
||||
- Risk: RapidAPI CLI command shape unknown. Mitigation: treat smoke command as operator-provided until verified.
|
||||
|
||||
## Security Considerations
|
||||
|
||||
- Never use `printenv`, `env`, or CLI debug flags to prove secret presence.
|
||||
- Validate by CLI success/failure and sanitized env key metadata.
|
||||
|
||||
## Next Steps
|
||||
|
||||
- If root cause is missing preset or diagnostics, Phase 2.
|
||||
- If root cause is stale cron payload only, Phase 3 documents remediation and issue reply.
|
||||
+133
@@ -0,0 +1,133 @@
|
||||
---
|
||||
phase: 2
|
||||
title: "Add RapidAPI Preset and Credential Diagnostics"
|
||||
status: complete
|
||||
priority: P1
|
||||
effort: "2.5h"
|
||||
dependencies: [1]
|
||||
---
|
||||
|
||||
# Phase 2: Add RapidAPI Preset and Credential Diagnostics
|
||||
|
||||
## Context Links
|
||||
|
||||
- Preset registry: `internal/tools/credential_presets.go`
|
||||
- SecureCLI execution: `internal/tools/credentialed_exec.go`
|
||||
- Env parsing/flattening: `internal/store/secure_cli_env.go`
|
||||
- UI consumes presets through existing SecureCLI preset endpoints.
|
||||
|
||||
## Overview
|
||||
|
||||
Add the smallest product support needed for RapidAPI cron use: a SecureCLI preset and precise diagnostics. Do not add RapidAPI-specific runtime branches.
|
||||
|
||||
## Key Insights
|
||||
|
||||
- SecureCLI presets are static templates only; adding `rapidapi` is low blast radius.
|
||||
- Credentialed exec logs currently show lookup found/no found, but not enough operator-safe reason detail.
|
||||
- Host fall-through exec scrubs credential env keys; that is correct and should not be bypassed.
|
||||
- 2026-05-29 implementation:
|
||||
- Added built-in `rapidapi` preset with required `RAPIDAPI_KEY`.
|
||||
- Added required preset env validation before binary resolution/execution.
|
||||
- Added safe env-key-only diagnostics for merged env and missing required keys.
|
||||
- Added `RAPIDAPI_KEY` to fall-through exec env scrub list.
|
||||
|
||||
## Requirements
|
||||
|
||||
- Functional: `rapidapi` appears in preset list with required `RAPIDAPI_KEY`.
|
||||
- Functional: credentialed exec logs safe metadata for lookup and injection attempt.
|
||||
- Functional: missing/empty merged env yields actionable error, not only downstream `RAPIDAPI_KEY required`.
|
||||
- Non-functional: no secret values in logs; key names allowed.
|
||||
- Non-functional: no cron API contract change.
|
||||
|
||||
## Architecture
|
||||
|
||||
Use existing SecureCLI:
|
||||
|
||||
```text
|
||||
Preset -> admin creates binary -> optional agent grant -> optional user env -> exec direct argv mode
|
||||
```
|
||||
|
||||
Diagnostics live at two layers:
|
||||
|
||||
- Lookup layer: found config? agent grant? credential user id present?
|
||||
- Injection layer: merged env key names count, required key missing, binary path resolution.
|
||||
|
||||
## Related Code Files
|
||||
|
||||
- Modify: `internal/tools/credential_presets.go`
|
||||
- Modify: `internal/tools/credentialed_exec.go`
|
||||
- Modify: tests near `internal/tools/credential*_test.go`
|
||||
- Modify if needed: `docs/project-changelog.md`
|
||||
- Avoid unless required: DB migrations, cron schema, HTTP API contracts.
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
### Tests Before
|
||||
|
||||
1. Add preset test:
|
||||
- `GetPreset("rapidapi")` exists.
|
||||
- env vars include required `RAPIDAPI_KEY`.
|
||||
- deny patterns avoid interactive/export/debug secret leakage if RapidAPI CLI has such commands.
|
||||
2. Add credentialed env validation test:
|
||||
- `rapidapi` config with no env returns an actionable missing-env error before executing binary.
|
||||
- `rapidapi` config with `RAPIDAPI_KEY` proceeds to binary resolution/execution path.
|
||||
3. Add logging behavior test only where stable without asserting log formatting too tightly.
|
||||
|
||||
### Refactor / Implementation
|
||||
|
||||
4. Add `rapidapi` preset:
|
||||
- `BinaryName: "rapidapi"`
|
||||
- `EnvVars: RAPIDAPI_KEY`
|
||||
- conservative timeout, e.g. 60s.
|
||||
- tips: use read-only/list/query commands; avoid debug output.
|
||||
5. Add optional required-env guard for preset-required keys if current SecureCLI model supports it without schema change.
|
||||
- If schema change would be needed, skip guard and rely on diagnostics/logging; YAGNI.
|
||||
6. Improve operator-safe diagnostics:
|
||||
- log binary, agent id, tenant id, credential user id presence, env key names only.
|
||||
- distinguish no credential row vs found row with empty env.
|
||||
- never log env values.
|
||||
7. Ensure non-credentialed fallback remains scrubbed and blocked for registered non-global binaries.
|
||||
|
||||
### Tests After
|
||||
|
||||
8. Run focused tests:
|
||||
- `go test ./internal/tools`
|
||||
- `go test ./internal/store`
|
||||
9. Run compile gate:
|
||||
- `go build ./...`
|
||||
|
||||
## Todo List
|
||||
|
||||
- [x] Preset regression test first.
|
||||
- [x] Missing-env diagnostic test first.
|
||||
- [x] Add minimal preset.
|
||||
- [x] Add safe diagnostics.
|
||||
- [x] Update changelog if code changes.
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [x] `rapidapi` preset available through existing preset API.
|
||||
- [x] Missing `RAPIDAPI_KEY` is distinguishable from policy block and binary-not-found.
|
||||
- [x] Logs expose only safe metadata.
|
||||
- [x] Existing SecureCLI security tests still pass.
|
||||
|
||||
## Validation
|
||||
|
||||
- `go test ./internal/tools -run 'RapidAPI|MissingRequiredEnv|MergeCredentialedEnv'`
|
||||
- `go test ./internal/tools`
|
||||
|
||||
## Risk Assessment
|
||||
|
||||
- Risk: required-env guard changes all SecureCLI behavior. Mitigation: only add if scoped and test-protected; otherwise skip.
|
||||
- Risk: logs leak identity detail. Mitigation: no values, no command args beyond existing truncation, key names only.
|
||||
- Risk: RapidAPI CLI binary name differs. Mitigation: verify command package before final implementation; if command is not `rapidapi`, update preset name to real binary.
|
||||
|
||||
## Security Considerations
|
||||
|
||||
- Keep direct argv execution for credentialed mode.
|
||||
- Do not support shell wrappers for RapidAPI credentials.
|
||||
- Deny verbose/debug options if RapidAPI CLI can print HTTP headers.
|
||||
|
||||
## Next Steps
|
||||
|
||||
- Phase 3 validates cron path end-to-end and prepares GitHub issue reply.
|
||||
+128
@@ -0,0 +1,128 @@
|
||||
---
|
||||
phase: 3
|
||||
title: "Validate Cron Smoke and Issue Handoff"
|
||||
status: complete
|
||||
priority: P1
|
||||
effort: "2h"
|
||||
dependencies: [2]
|
||||
---
|
||||
|
||||
# Phase 3: Validate Cron Smoke and Issue Handoff
|
||||
|
||||
## Context Links
|
||||
|
||||
- Cron methods: `internal/gateway/methods/cron.go`
|
||||
- Cron handler: `cmd/gateway_cron.go`
|
||||
- Cron docs: `docs/08-scheduling-cron.md`
|
||||
- SecureCLI docs: `docs/03-tools-system.md`
|
||||
|
||||
## Overview
|
||||
|
||||
Validate cron-triggered RapidAPI usage through tests and one operator-approved smoke path. Then update docs/changelog and reply to issue #74 with exact result and plan path.
|
||||
|
||||
## Key Insights
|
||||
|
||||
- Existing tests already cover cron credential context injection.
|
||||
- #74 acceptance asks for smoke test and clear logs.
|
||||
- Live smoke may need real RapidAPI key; no fake credential should be used to claim success.
|
||||
- 2026-05-29 validation:
|
||||
- Cron credential context injection and redaction tests are already present and covered by targeted test runs.
|
||||
- RapidAPI direct exec is covered with a local script fixture and injected `RAPIDAPI_KEY`.
|
||||
- Real RapidAPI cron smoke was not run because no operator-approved key, target agent grant, or harmless endpoint was provided in this session.
|
||||
|
||||
## Requirements
|
||||
|
||||
- Functional: cron-created-after-fix can access configured RapidAPI credential through SecureCLI.
|
||||
- Functional: missing credentials return actionable error.
|
||||
- Functional: GitHub issue gets concise summary and plan path.
|
||||
- Non-functional: no secret printing or durable secret artifacts.
|
||||
- Non-functional: skip load/stress tests per repo rule.
|
||||
|
||||
## Architecture
|
||||
|
||||
Validation combines:
|
||||
|
||||
- unit tests for context and env wiring,
|
||||
- optional integration/manual smoke for real RapidAPI CLI,
|
||||
- issue handoff comment with branch/plan path.
|
||||
|
||||
## Related Code Files
|
||||
|
||||
- Modify if implementation occurred: `docs/project-changelog.md`
|
||||
- Modify if docs need command examples: `docs/03-tools-system.md`
|
||||
- Test: `cmd/gateway_cron_test.go`
|
||||
- Test: `internal/tools/credentialed_exec*_test.go`
|
||||
- Test: `internal/store/*secure_cli*_test.go`
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
### Tests Before
|
||||
|
||||
1. Add/confirm regression proving cron context carries `credentialUserId` into scheduler run.
|
||||
2. Add test proving redacted cron API response never exposes `credentialUserId`.
|
||||
3. Add test proving RapidAPI env key can be merged for credentialed exec without exposing value.
|
||||
|
||||
### Validation
|
||||
|
||||
4. Run targeted tests:
|
||||
- `go test ./cmd ./internal/tools ./internal/store`
|
||||
5. Run build gates if code changed:
|
||||
- `go build ./...`
|
||||
- `go build -tags sqliteonly ./...`
|
||||
6. If real RapidAPI credentials available and user approves:
|
||||
- create/update SecureCLI `rapidapi` config with sanitized workflow.
|
||||
- grant target agent.
|
||||
- create fresh cron job after #54 fix.
|
||||
- run harmless RapidAPI read command.
|
||||
- verify run log success and no secret output.
|
||||
7. If live credentials unavailable:
|
||||
- document skipped live smoke explicitly.
|
||||
- keep issue open or mark plan blocked by credential availability.
|
||||
|
||||
### Handoff
|
||||
|
||||
8. Update changelog/docs only if implementation changes behavior.
|
||||
9. Reply GitHub issue #74:
|
||||
- plan path,
|
||||
- root-cause hypothesis,
|
||||
- chosen approach,
|
||||
- validation gates,
|
||||
- open operator questions.
|
||||
|
||||
## Todo List
|
||||
|
||||
- [x] Regression tests pass.
|
||||
- [x] Build gates pass if code changed.
|
||||
- [x] Smoke result recorded or skipped with reason.
|
||||
- [ ] Issue #74 replied with plan summary and filepath after PR is opened.
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [x] Tests prove cron and SecureCLI credential path for RapidAPI.
|
||||
- [x] Manual smoke either passes or is clearly blocked by missing real credentials.
|
||||
- [x] No secrets in logs, tests, docs, or issue comment.
|
||||
- [ ] GitHub issue comment links this plan and summarizes next implementation path after PR is opened.
|
||||
|
||||
## Validation
|
||||
|
||||
- `go test ./cmd ./internal/tools ./internal/store`
|
||||
- `go build ./...`
|
||||
- `go build -tags sqliteonly ./...`
|
||||
- `go vet ./...`
|
||||
- `TEST_DATABASE_URL="postgres://postgres:test@localhost:5433/goclaw_test?sslmode=disable" go test -race -tags integration ./tests/integration/`
|
||||
|
||||
## Risk Assessment
|
||||
|
||||
- Risk: smoke uses fake data and gives false confidence. Mitigation: fake keys only test error wording; real smoke must use real read-only endpoint.
|
||||
- Risk: issue comment overstates completion. Mitigation: label plan as pending until implementation/smoke complete.
|
||||
- Risk: docs drift from actual CLI syntax. Mitigation: verify RapidAPI CLI command syntax before adding command examples.
|
||||
|
||||
## Security Considerations
|
||||
|
||||
- Do not paste RapidAPI key in commands.
|
||||
- Do not use command forms that echo env.
|
||||
- Keep issue comment plan-only, no live config values.
|
||||
|
||||
## Next Steps
|
||||
|
||||
- After plan approval, run `/ck:cook /Volumes/GOON/www/digitop/goclaw/plans/260528-1801-rapidapi-cron-securecli-credentials/plan.md --tdd`.
|
||||
@@ -0,0 +1,69 @@
|
||||
---
|
||||
title: "RapidAPI cron SecureCLI credential fix"
|
||||
description: "Diagnose and fix RapidAPI CLI credential injection from cron-triggered agent turns without changing cron credential architecture."
|
||||
status: complete
|
||||
priority: P1
|
||||
effort: 6h
|
||||
issue: 74
|
||||
branch: "codex/issue-74-rapidapi-cron-plan"
|
||||
tags: [bugfix, backend, security, cron, secure-cli, tdd]
|
||||
blockedBy: []
|
||||
blocks: []
|
||||
created: "2026-05-28T11:01:28.893Z"
|
||||
createdBy: "ck:plan"
|
||||
source: skill
|
||||
---
|
||||
|
||||
# RapidAPI cron SecureCLI credential fix
|
||||
|
||||
## Overview
|
||||
|
||||
Issue #74 reports `rapidapi` CLI failing from cron with `RAPIDAPI_KEY required`.
|
||||
Broader cron credential context is already fixed by storing `payload.credentialUserId` on create and injecting it into cron runs. This plan keeps that architecture and focuses on RapidAPI-specific SecureCLI setup, diagnostics, and regression smoke.
|
||||
|
||||
## Phases
|
||||
|
||||
| Phase | Name | Status |
|
||||
|-------|------|--------|
|
||||
| 1 | [Diagnose Current RapidAPI Credential Path](./phase-01-diagnose-current-rapidapi-credential-path.md) | Complete |
|
||||
| 2 | [Add RapidAPI Preset and Credential Diagnostics](./phase-02-add-rapidapi-preset-and-credential-diagnostics.md) | Complete |
|
||||
| 3 | [Validate Cron Smoke and Issue Handoff](./phase-03-validate-cron-smoke-and-issue-handoff.md) | Complete |
|
||||
|
||||
## Dependencies
|
||||
|
||||
- GitHub issue: `digitopvn/goclaw#74`
|
||||
- Related closed issue: `digitopvn/goclaw#54`
|
||||
- Cron context path: `cmd/gateway_cron.go`, `internal/store/cron_store.go`, `internal/gateway/methods/cron.go`
|
||||
- SecureCLI path: `internal/tools/credentialed_exec.go`, `internal/tools/credential_presets.go`, `internal/store/secure_cli_store.go`
|
||||
- Docs to update if behavior changes: `docs/project-changelog.md`, `docs/03-tools-system.md`
|
||||
|
||||
## Recommended Scope
|
||||
|
||||
- First prove whether live config is missing `rapidapi`, missing agent grant, missing user env, or legacy cron payload.
|
||||
- Add built-in RapidAPI preset only if config gap confirmed or reproducible from local tests.
|
||||
- Add logs/tests that distinguish: no SecureCLI config, no grant, no `RAPIDAPI_KEY`, invalid env JSON, binary not found.
|
||||
|
||||
## Out of Scope
|
||||
|
||||
- New credential store.
|
||||
- New cron auth model.
|
||||
- Exposing credential values in API/logs.
|
||||
- Load/stress tests.
|
||||
- Direct live write smoke unless operator provides explicit credentialed environment.
|
||||
|
||||
## Validation Gates
|
||||
|
||||
- `go test ./internal/tools ./internal/store ./cmd`
|
||||
- Targeted cron/SecureCLI regression tests before implementation changes.
|
||||
- Manual or documented smoke: cron-created-after-fix invokes harmless `rapidapi` command and does not print secret.
|
||||
|
||||
## Completion Notes
|
||||
|
||||
- 2026-05-29: all code/test/build gates passed, including PG and SQLite builds, `go vet`, and integration race suite with local PG test DB.
|
||||
- Real RapidAPI cron smoke remains operator-gated because this session did not include an approved RapidAPI key, target SecureCLI grant, or harmless endpoint.
|
||||
|
||||
## Unresolved Questions
|
||||
|
||||
- Is `rapidapi` registered in the live SecureCLI table now?
|
||||
- Was the failing cron created before `v3.12.0-beta.25` / commit `f9440bac`?
|
||||
- Which harmless RapidAPI endpoint should be the operator-approved smoke target?
|
||||
Reference in new issue
Block a user