From 80c4d16c909907ab740464b18a53dd20fe3bca65 Mon Sep 17 00:00:00 2001 From: jettwang Date: Mon, 28 Sep 2026 11:18:35 +0800 Subject: [PATCH] fix(cli): align diagnostics and clear scan findings Fix run JSON admission reporting, add nested sudo guidance for scripts, and make SQL database-name validation errors actionable. Document the v0.19.1 patch release and use gosec-native suppressions for reviewed scanner false positives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 16 +++++- README.md | 5 ++ README_CN.md | 4 ++ internal/app/lifecycle.go | 20 ++++++- internal/app/lifecycle_policy_test.go | 30 ++++++++++ internal/app/password.go | 2 +- internal/app/run.go | 9 ++- internal/app/run_sudo_notice_test.go | 81 +++++++++++++++++++++++++++ internal/app/usage_sections.go | 2 + internal/app/usage_test.go | 2 + internal/plugin/store.go | 3 +- internal/skillinstall/install.go | 3 +- internal/sqlsafe/postgres.go | 11 +++- internal/sqlsafe/sqlsafe_test.go | 21 +++++++ internal/sshclient/validate.go | 7 +++ internal/sshclient/validate_test.go | 24 ++++++++ pkg/logger/logger.go | 2 +- 17 files changed, 232 insertions(+), 10 deletions(-) create mode 100644 internal/app/run_sudo_notice_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 52d555d..f9fb432 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.19.1] - 2026-09-28 + +### Fixed + +- `sshx run --json` mirrors the structured reason for safety-policy blocks to + stderr and reports the block as `phase=admission`, matching command mode. +- `sshx run --script-file` and `--script-stdin` warn when nested `sudo` is + present without `--sudo`, explaining that the script occupies stdin and how + to run the interpreter privileged. +- `sshx sql` database-name validation errors state the allowed characters and + identify bracketed uppercase values that look like redacted placeholders. + ## [0.19.0] - 2026-09-24 ### Added @@ -833,7 +845,9 @@ diagnostics). - CI/CD workflow and automated release process - Tag creation script -[Unreleased]: https://github.com/talkincode/sshx/compare/v0.18.0...HEAD +[Unreleased]: https://github.com/talkincode/sshx/compare/v0.19.1...HEAD +[0.19.1]: https://github.com/talkincode/sshx/compare/v0.19.0...v0.19.1 +[0.19.0]: https://github.com/talkincode/sshx/compare/v0.18.0...v0.19.0 [0.18.0]: https://github.com/talkincode/sshx/compare/v0.17.0...v0.18.0 [0.17.0]: https://github.com/talkincode/sshx/compare/v0.16.1...v0.17.0 [0.16.1]: https://github.com/talkincode/sshx/compare/v0.16.0...v0.16.1 diff --git a/README.md b/README.md index 134e1eb..6a60a76 100644 --- a/README.md +++ b/README.md @@ -332,6 +332,11 @@ sshx run --group=prod-web --tag=env=prod --concurrency=4 --jsonl -- "uptime" sshx run --target=prod-web --script-file=./check.sh --json ``` +For script payloads, `--sudo` runs the selected interpreter with privilege. If +sshx detects a nested `sudo` command without `--sudo`, it writes a notice to +stderr before connecting: the script occupies stdin, so sshx cannot inject a +stored sudo password into that nested process. `--quiet` suppresses the notice. + Multi-target exit codes: `0` all succeeded, `1` partial failure/skip/uncertain, `255` request-level failure (invalid selectors, zero matches, bad input). diff --git a/README_CN.md b/README_CN.md index 7c9f7f1..be09b3d 100644 --- a/README_CN.md +++ b/README_CN.md @@ -319,6 +319,10 @@ sshx run --group=prod-web --tag=env=prod --concurrency=4 --jsonl -- "uptime" sshx run --target=prod-web --script-file=./check.sh --json ``` +脚本模式下，`--sudo` 会以 sudo 运行所选解释器。若 sshx 检测到脚本含有嵌套 +`sudo` 命令但未指定 `--sudo`，会在连接前向 stderr 提示：脚本占用了 stdin， +sshx 无法向嵌套进程注入已保存的 sudo 密码。`--quiet` 可以静默此提示。 + 多主机退出码：`0` 全部成功，`1` 部分失败/跳过/不确定，`255` 请求级失败。 ### `--dry-run` 执行计划预览 diff --git a/internal/app/lifecycle.go b/internal/app/lifecycle.go index 85c4ccb..07e00e3 100644 --- a/internal/app/lifecycle.go +++ b/internal/app/lifecycle.go @@ -158,7 +158,7 @@ func reportPolicyRejection(w io.Writer, document map[string]json.RawMessage) { } writeDiagnosticNote(w, "sshx: blocked by safety policy (phase=%s, error_kind=%s, executed=%s, exit_code=%d); no remote command ran\n", phase, execution.ErrorKindBlocked, executed, exitCode) - if reason := flattenPolicyReason(documentString(document, "error")); reason != "" { + if reason := flattenPolicyReason(documentErrorMessage(document)); reason != "" { writeDiagnosticNote(w, "sshx: block reason: %s\n", reason) } } @@ -184,6 +184,24 @@ func documentString(document map[string]json.RawMessage, key string) string { return text } +func documentErrorMessage(document map[string]json.RawMessage) string { + raw, ok := document["error"] + if !ok { + return "" + } + var message string + if err := json.Unmarshal(raw, &message); err == nil { + return message + } + var info struct { + Message string `json:"message"` + } + if err := json.Unmarshal(raw, &info); err != nil { + return "" + } + return info.Message +} + func finalizeLifecycle(config *sshclient.Config, value any) (map[string]json.RawMessage, error) { data, err := marshalLifecycleValue(value) if err != nil { diff --git a/internal/app/lifecycle_policy_test.go b/internal/app/lifecycle_policy_test.go index e1995d3..69eb2b5 100644 --- a/internal/app/lifecycle_policy_test.go +++ b/internal/app/lifecycle_policy_test.go @@ -156,6 +156,36 @@ func TestBlockedJSONKeepsStdoutPureAndMirrorsStderr(t *testing.T) { assert.NotContains(t, string(stdout), "blocked by safety policy", "the mirror must not leak into stdout") } +func TestRunJSONBlockedAdmissionMirrorsReason(t *testing.T) { + setTestHome(t, t.TempDir()) + + args := []string{ + "sshx", "run", "--address=192.0.2.1", "--json", "--no-audit", "--", + `docker exec db psql -U app -c 'SELECT 1'`, + } + var runErr error + stdout, stderr := captureStreams(t, func() { + runErr = Run(args) + }) + require.ErrorIs(t, runErr, ErrReported) + + var document struct { + Phase string `json:"phase"` + Error struct { + Kind string `json:"kind"` + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(stdout, &document), string(stdout)) + assert.Equal(t, "admission", document.Phase) + assert.Equal(t, "blocked", document.Error.Kind) + assert.Contains(t, document.Error.Message, "bypasses the guarded SQL pipeline") + assert.Contains(t, string(stderr), "phase=admission") + assert.Contains(t, string(stderr), "sshx: block reason:") + assert.Contains(t, string(stderr), "bypasses the guarded SQL pipeline") + assert.Contains(t, string(stderr), "sshx sql") +} + // TestBlockedJSONMirrorIsSilencedByQuiet: --quiet keeps the same single JSON // document on stdout and drops the human mirror from stderr, so a caller that // merges the streams still reads exactly one parseable document (issue #86). diff --git a/internal/app/password.go b/internal/app/password.go index e3e48c1..39e91f3 100644 --- a/internal/app/password.go +++ b/internal/app/password.go @@ -16,7 +16,7 @@ import ( "github.com/talkincode/sshx/pkg/logger" ) -const secretsSchemaVersion = "sshx.secrets.v1" //nolint:gosec // schema id, not a credential +const secretsSchemaVersion = "sshx.secrets.v1" // #nosec G101 -- fixed schema identifier, not a credential // errPasswordNotFound is returned by --password-check when the named key is // absent. Callers that branch on exit code must treat this as failure. diff --git a/internal/app/run.go b/internal/app/run.go index e8cbabd..912a021 100644 --- a/internal/app/run.go +++ b/internal/app/run.go @@ -246,12 +246,16 @@ func reportRunRequestFailure(config *sshclient.Config, audit *auditRecorder, err } meta := execution.NewMetadata(plan, config.ExecutionID) attachPrepared(config, &preparedOperation{plan: plan, meta: meta, audit: audit}) + phase := execution.PhaseResolve + if kind == execution.ErrorKindBlocked { + phase = execution.PhaseAdmission + } res := execution.Result{ Metadata: meta, SchemaVersion: execution.ResultSchemaVersion, RunID: config.ExecutionID, Status: execution.StatusFailed, - Phase: execution.PhaseResolve, + Phase: phase, Completion: execution.CompletionNotStarted, ExitCode: -1, Success: false, @@ -463,6 +467,9 @@ func buildRunRequest(config *sshclient.Config) (*execution.Request, *execution.P if runner := runnerFromConfig(config, payload); runner != "" { req.Action.ScriptRunner = runner } + if !req.Action.UseSudo && sshclient.ContainsSudoCommand(string(payload.Bytes)) { + writeDiagnosticNote(noticeWriter(config), "sshx: the script contains `sudo` but --sudo was not requested; sshx cannot inject a password for a nested sudo (its stdin carries the script). Re-run with --sudo to run the interpreter privileged, or remove the sudo calls.\n") + } } return req, payload, nil diff --git a/internal/app/run_sudo_notice_test.go b/internal/app/run_sudo_notice_test.go new file mode 100644 index 0000000..eeaf485 --- /dev/null +++ b/internal/app/run_sudo_notice_test.go @@ -0,0 +1,81 @@ +package app + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRunScriptSudoNotice(t *testing.T) { + setTestHome(t, t.TempDir()) + + tests := []struct { + name string + script string + sudo bool + quiet bool + want bool + }{ + { + name: "nested sudo", + script: "#!/bin/sh\nsudo -n true\n", + want: true, + }, + { + name: "sudo requested", + script: "#!/bin/sh\nsudo -n true\n", + sudo: true, + }, + { + name: "quiet", + script: "#!/bin/sh\nsudo -n true\n", + quiet: true, + }, + { + name: "sudo is only an argument", + script: "#!/bin/sh\necho sudo\n", + }, + { + name: "sudo is only a comment", + script: "#!/bin/sh\n# sudo reboot\necho ready\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scriptPath := filepath.Join(t.TempDir(), "script.sh") + require.NoError(t, os.WriteFile(scriptPath, []byte(tt.script), 0o600)) + + args := []string{ + "sshx", "run", "--address=192.0.2.1", "--script-file=" + scriptPath, + "--dry-run", "--json", "--no-audit", + } + if tt.sudo { + args = append(args, "--sudo") + } + if tt.quiet { + args = append(args, "--quiet") + } + + var runErr error + stdout, stderr := captureStreams(t, func() { + runErr = Run(args) + }) + require.NoError(t, runErr) + assert.True(t, len(stdout) > 0, "dry-run should retain its JSON document") + + noticeCount := strings.Count(string(stderr), "the script contains `sudo`") + if tt.want { + assert.Equal(t, 1, noticeCount, "notice should be written once") + assert.Contains(t, string(stderr), "its stdin carries the script") + assert.Contains(t, string(stderr), "Re-run with --sudo") + } else { + assert.Zero(t, noticeCount) + } + }) + } +} diff --git a/internal/app/usage_sections.go b/internal/app/usage_sections.go index 9ae1062..f05fb02 100644 --- a/internal/app/usage_sections.go +++ b/internal/app/usage_sections.go @@ -90,6 +90,8 @@ Run Contract (preferred for Agents): --sudo runs the whole script interpreter with sudo. Do not embed sudo commands in a script: its stdin is occupied by the script, so sshx cannot inject the password for a nested sudo command. + If sshx detects nested sudo without --sudo, it warns before connecting; + --quiet suppresses this notice. Limits / policy: --concurrency=N default 4, hard max 32 diff --git a/internal/app/usage_test.go b/internal/app/usage_test.go index c2b2ed2..b15912e 100644 --- a/internal/app/usage_test.go +++ b/internal/app/usage_test.go @@ -245,6 +245,8 @@ func TestRunHelpExplainsSudoScriptPayloads(t *testing.T) { require.Contains(t, output, "Do not embed sudo") require.Contains(t, output, "its stdin is occupied by the script") require.Contains(t, output, "inject the password for a nested sudo command") + require.Contains(t, output, "it warns before connecting") + require.Contains(t, output, "--quiet suppresses this notice") } func TestSQLHelpDocumentsFullTableBackupOptIn(t *testing.T) { diff --git a/internal/plugin/store.go b/internal/plugin/store.go index f00b977..7faee60 100644 --- a/internal/plugin/store.go +++ b/internal/plugin/store.go @@ -53,7 +53,8 @@ func ensurePrivateRoot(root string) error { if err := os.MkdirAll(root, 0o700); err != nil { return fmt.Errorf("create plugin root: %w", err) } - if err := os.Chmod(root, 0o700); err != nil { // #nosec G302 -- private directory requires owner traversal. + // #nosec G302 -- directories require execute bits for owner traversal. + if err := os.Chmod(root, 0o700); err != nil { return fmt.Errorf("secure plugin root: %w", err) } return nil diff --git a/internal/skillinstall/install.go b/internal/skillinstall/install.go index f984e65..fbb1f73 100644 --- a/internal/skillinstall/install.go +++ b/internal/skillinstall/install.go @@ -160,7 +160,8 @@ func ensureTargetDir(dir string) error { return fmt.Errorf("inspect skill directory %s: %w", dir, err) } - if mkdirErr := os.MkdirAll(dir, 0o755); mkdirErr != nil { // #nosec G301 -- Agent skills are public documentation, not secrets. + // #nosec G301 -- Agent skills are public documentation, not secrets. + if mkdirErr := os.MkdirAll(dir, 0o755); mkdirErr != nil { return fmt.Errorf("create skill directory %s: %w", dir, mkdirErr) } info, err = os.Lstat(dir) diff --git a/internal/sqlsafe/postgres.go b/internal/sqlsafe/postgres.go index 9319766..2b89c68 100644 --- a/internal/sqlsafe/postgres.go +++ b/internal/sqlsafe/postgres.go @@ -312,12 +312,17 @@ func ValidateTableIdent(table string) error { // ValidateDatabaseName bounds the database name used in argv and paths. var databaseNameRE = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) +var redactedDatabaseNameRE = regexp.MustCompile(`^\[[A-Z][A-Z0-9 _-]*\]$`) func ValidateDatabaseName(name string) error { - if !databaseNameRE.MatchString(name) { - return &BlockedError{Reason: fmt.Sprintf("database name %q contains unsupported characters", name)} + if databaseNameRE.MatchString(name) { + return nil } - return nil + reason := fmt.Sprintf("database name %q contains unsupported characters; allowed: [A-Za-z0-9_-]", name) + if redactedDatabaseNameRE.MatchString(name) { + reason += "; value looks like a redacted placeholder; pass the real database name" + } + return &BlockedError{Reason: reason} } // ValidateBackupDir rejects control characters that could escape psql diff --git a/internal/sqlsafe/sqlsafe_test.go b/internal/sqlsafe/sqlsafe_test.go index c738a6f..09db72e 100644 --- a/internal/sqlsafe/sqlsafe_test.go +++ b/internal/sqlsafe/sqlsafe_test.go @@ -484,6 +484,27 @@ func TestValidateTableIdent(t *testing.T) { } } +func TestValidateDatabaseNameExplainsAllowedCharactersAndPlaceholders(t *testing.T) { + for _, name := range []string{"app", "app_db", "app-db", "db123"} { + assert.NoError(t, ValidateDatabaseName(name), name) + } + + err := ValidateDatabaseName("my db;drop") + require.Error(t, err) + assert.Contains(t, err.Error(), "allowed: [A-Za-z0-9_-]") + assert.NotContains(t, err.Error(), "redacted placeholder") + + placeholderErr := ValidateDatabaseName("[REDACTED]") + require.Error(t, placeholderErr) + assert.Contains(t, placeholderErr.Error(), "allowed: [A-Za-z0-9_-]") + assert.Contains(t, placeholderErr.Error(), "looks like a redacted placeholder") + assert.Contains(t, placeholderErr.Error(), "pass the real database name") + + lowercaseErr := ValidateDatabaseName("[redacted]") + require.Error(t, lowercaseErr) + assert.NotContains(t, lowercaseErr.Error(), "redacted placeholder") +} + func TestValidateBackupDir(t *testing.T) { assert.NoError(t, ValidateBackupDir(".sshx/sql-backups")) assert.NoError(t, ValidateBackupDir("/tmp/operator's backups")) diff --git a/internal/sshclient/validate.go b/internal/sshclient/validate.go index 0943e30..a52c485 100644 --- a/internal/sshclient/validate.go +++ b/internal/sshclient/validate.go @@ -97,6 +97,13 @@ func CommandUsesSudo(command string) bool { return ok } +// ContainsSudoCommand reports whether any shell segment runs sudo as its +// command. It is used for script payloads, where even a leading sudo reads from +// stdin and cannot use sshx's password auto-fill. +func ContainsSudoCommand(command string) bool { + return sudoInCommandPosition(strings.TrimSpace(command), 0) +} + func leadingSudoRemainder(command string) (string, bool) { trimmed := strings.TrimLeft(command, " \t\r\n") if trimmed == "sudo" { diff --git a/internal/sshclient/validate_test.go b/internal/sshclient/validate_test.go index 0a9a328..b6b71ce 100644 --- a/internal/sshclient/validate_test.go +++ b/internal/sshclient/validate_test.go @@ -492,3 +492,27 @@ func TestCommandUsesSudo(t *testing.T) { }) } } + +func TestContainsSudoCommand(t *testing.T) { + tests := []struct { + name string + command string + want bool + }{ + {"script starts with sudo", "#!/bin/sh\nsudo -n true\n", true}, + {"sudo in later shell segment", "echo ready && sudo id -u", true}, + {"sudo in shell wrapper", "sh -c 'sudo whoami'", true}, + {"sudo as argument", "echo sudo", false}, + {"sudo in a comment", "# sudo reboot\necho ready", false}, + {"sudo inside container command", "docker exec db sudo id", false}, + {"no sudo", "id -u", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ContainsSudoCommand(tt.command); got != tt.want { + t.Errorf("ContainsSudoCommand(%q) = %v, want %v", tt.command, got, tt.want) + } + }) + } +} diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index e94311c..3b8b422 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -139,7 +139,7 @@ func (l *Logger) EnableFileLogging(logPath string) error { } // 打开日志文件 - file, err := os.OpenFile(logPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) //nolint:gosec // controlled log path + file, err := os.OpenFile(logPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) // #nosec G304 -- explicit caller-selected log destination; the CLI uses ~/.sshx/sshx.log if err != nil { return fmt.Errorf("failed to open log file: %w", err) }