From e786565e061dc3a3a452379eb0a2c8c4efd4cd9d Mon Sep 17 00:00:00 2001 From: Mira Voss Date: Thu, 24 Sep 2026 11:39:41 +0800 Subject: [PATCH] fix: guard SQL backup scope and apply permissions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 23 +++++ README.md | 10 ++- SECURITY.md | 4 +- docs/agent-scripting.md | 1 + docs/apply.md | 8 ++ docs/contract.md | 2 +- docs/mcp.md | 5 ++ docs/zh/agent-scripting.md | 1 + docs/zh/apply.md | 5 ++ docs/zh/mcp.md | 4 + internal/app/app.go | 1 + internal/app/apply.go | 15 ++-- internal/app/cli_surface_test.go | 11 +++ internal/app/config.go | 10 ++- internal/app/dryrun.go | 50 ++++++----- internal/app/lifecycle.go | 12 +++ internal/app/mcp.go | 58 +++++++------ internal/app/mcp_test.go | 22 ++--- internal/app/sql.go | 47 ++++++---- internal/app/sql_test.go | 34 +++++++- internal/app/usage_sections.go | 28 ++++-- internal/app/usage_test.go | 28 ++++++ internal/sqlsafe/dialect.go | 1 + internal/sqlsafe/policy.go | 57 +++++++++++-- internal/sqlsafe/sqlite.go | 35 +++++--- internal/sqlsafe/sqlite_test.go | 12 ++- internal/sqlsafe/sqlsafe_test.go | 10 ++- internal/sshclient/apply.go | 69 +++++++++++++-- internal/sshclient/apply_fault_test.go | 12 +-- internal/sshclient/client.go | 4 + tests/e2e/apply_e2e_test.go | 57 +++++++++++++ tests/e2e/harness_test.go | 44 ++++++++++ tests/e2e/sql_mysql_e2e_test.go | 29 ++++++- tests/e2e/sql_sqlite_e2e_test.go | 113 ++++++++++++++++++++++++- 34 files changed, 684 insertions(+), 138 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fe149d..52d555d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.19.0] - 2026-09-24 + +### Added + +- `sshx sql --target=` as an alias for `-h` / `--host`, matching the + target selector used by `run` and `apply`. +- `sshx sql --allow-full-table-backup` as an explicit opt-in for row-filtered + mutations whose before-image must include the full table. + +### Changed + +- Row-filtered SQL mutations now use a narrow before-image when the selected + rows can be safely reproduced. If backup planning must widen to a full-table + snapshot, execution is blocked by default with a stable reason code; dry-run + and JSON results expose the planned backup scope. +- `sshx apply` checks parent-directory write and execute permission before + creating backup or temporary files, returning + `parent_directory_not_writable` and suggesting `--sudo` when a sudo + credential is configured. +- `sshx run --help` distinguishes `--sudo` for the selected script interpreter + from nested `sudo` commands in the script, whose stdin prevents password + injection. + ## [0.18.0] - 2026-09-23 ### Added diff --git a/README.md b/README.md index 4696b89..94dfeb1 100644 --- a/README.md +++ b/README.md @@ -470,8 +470,14 @@ printf '%s' 'SELECT count(*) FROM users' | sshx sql -h=prod-db --db=app --json `--allow-full-table`. Destructive DDL requires `--force --no-backup`; sshx does not claim an automatic restorable backup for schema destruction. Skipping a DML backup also requires both `--no-backup` and `--force`. Small changes receive a -row CSV snapshot; complex or large changes receive a full-table CSV snapshot -under `~/.sshx/sql-backups/`. Backup and mutation run in one PostgreSQL +row CSV snapshot. If a row-filtered mutation cannot be backed up narrowly, or +the EXPLAIN estimate exceeds `--row-threshold`, the full-table before-image is +blocked by default (`unreproducible_select` or +`full_table_backup_requires_opt_in`). Pass `--allow-full-table-backup` to +explicitly permit that wider backup; it does not replace `--allow-full-table` +for a mutation without `WHERE`. SQLite likewise uses row CSV for stable +predicates and blocks an unreproducible full-table fallback by default. +Backups land under `~/.sshx/sql-backups/`. Backup and mutation run in one PostgreSQL transaction while holding a target-table write lock, closing the concurrency window between them. Catalog preflight blocks automatic execution when triggers, rewrite rules, partitions, or cascading referential actions can diff --git a/SECURITY.md b/SECURITY.md index 20a13fb..f80c85a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -6,9 +6,9 @@ We take security seriously. The following versions of SSHX are currently support | Version | Supported | | -------- | ------------------ | +| 0.19.x | :white_check_mark: | | 0.18.x | :white_check_mark: | -| 0.17.x | :white_check_mark: | -| < 0.17.0 | :x: | +| < 0.18.0 | :x: | Security updates are provided for the latest minor release and the previous minor release (N-1). Older lines do not receive patches; please upgrade. diff --git a/docs/agent-scripting.md b/docs/agent-scripting.md index c62c469..769eee2 100644 --- a/docs/agent-scripting.md +++ b/docs/agent-scripting.md @@ -15,6 +15,7 @@ cat ./check.sh | sshx run --target=prod-web --script-stdin --json - Selectors resolve configured hosts only. Use `--address=` for one literal address. - Script payloads are streamed on SSH stdin and are not reconstructed through shell joining. +- `--sudo` runs the selected script interpreter as a whole via sudo. Do not embed sudo commands in a script: the script occupies stdin, so sshx cannot inject the password for nested sudo. - The script's `#!` line selects the interpreter, so a `#!/usr/bin/env bash` payload keeps bash semantics (`set -o pipefail`, arrays, `[[ ]]`). Use `--shell=NAME` to override it. Supported: `sh`, `bash`, `zsh`, `dash`, `ksh`, `ash`; any other interpreter is rejected as `error_kind: config` without connecting. The choice appears as `action.script_runner`. - Dry-run and results expose payload SHA-256 and byte length, not raw script contents. - Multi-target `--jsonl` streams `run_started`, per-target events, and `run_finished`. diff --git a/docs/apply.md b/docs/apply.md index d588f74..68358be 100644 --- a/docs/apply.md +++ b/docs/apply.md @@ -60,6 +60,14 @@ approval meanings. It cannot bypass `--expect-plan`. SFTP runs as the SSH user. Use `--sudo` when the target is not writable by that user. sshx stages the payload under the remote home directory, then runs a privileged stdin script to install it. The script is never left on the host. +Before writing a backup or replacement temp, `apply` checks that the target's +parent directory is writable and searchable by the SSH user. Atomic replacement +requires both write and execute permission on that directory, regardless of +the target file's owner. A denial returns +`error_kind: parent_directory_not_writable`; when a sudo password key is +configured, the error suggests `--sudo`. This preflight cannot prevent a +concurrent permission change after the check. + ```bash sshx apply --target=prod-web --path=/etc/nginx/nginx.conf \ --from=./nginx.conf --sudo --json diff --git a/docs/contract.md b/docs/contract.md index f59af3f..a4bbfea 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -22,7 +22,7 @@ renaming, or changing the meaning of a field, flag, exit code, or | Text dissection | `sshx.text.v1` | `sshx text --json`; help document is `sshx.text.help.v1` | | Exit codes | `0`, `1..254`, `255` | Remote status vs sshx-level failure | | JSON sshx failure | `exit_code: -1` | Distinguishes a remote `exit 255` | -| `error_kind` | `timeout`, `auth`, `host_key`, `connect`, `blocked`, `exit_missing`, `config`, `error`, plus SQL/apply additions | Branch on this field, not prose | +| `error_kind` | `timeout`, `auth`, `host_key`, `connect`, `blocked`, `exit_missing`, `config`, `error`, plus SQL/apply additions including `unreproducible_select`, `full_table_backup_requires_opt_in`, and `parent_directory_not_writable` | Branch on this field, not prose | | JSONL event types | `run_started`, `target_started`, `target_finished`, `run_finished` | | CLI flags listed in `sshx --help` for a released minor version remain valid diff --git a/docs/mcp.md b/docs/mcp.md index 95178ff..4d1a4b3 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -42,6 +42,11 @@ Claude Desktop / generic MCP client entry: | `sshx_transfer` | `--transfer` | Server-to-server streaming through the local machine | | `sshx_host_list` | `--host-list --json` | Read-only `sshx.hosts.v1` inventory | +For row-filtered SQL, a backup that would widen to a full-table snapshot is +blocked by default. Set `allow_full_table_backup: true` on `sshx_sql` only when +that broader before-image is intentional; this does not authorize an +`UPDATE`/`DELETE` without `WHERE`. + Tool results contain the CLI's versioned JSON verbatim (for example `sshx.result.v1` from `sshx_run`), so `success`, `error_kind`, `completion`, and retry guidance keep exactly the semantics documented for the CLI. A diff --git a/docs/zh/agent-scripting.md b/docs/zh/agent-scripting.md index deaf6cc..4bda4bd 100644 --- a/docs/zh/agent-scripting.md +++ b/docs/zh/agent-scripting.md @@ -14,6 +14,7 @@ sshx run --target=prod-web --script-file=./check.sh --dry-run --json - 选择器只解析已配置主机；字面地址用 `--address=`，不能进入 group/tag 扩散。 - 脚本经 SSH stdin 原样传输，不经本地 `strings.Join` 拼装。 +- `--sudo` 会把选定的脚本解释器整体交给 sudo 运行。不要在脚本里再嵌套 sudo 命令：脚本占用了 stdin，sshx 无法再向嵌套 sudo 注入密码。 - 脚本的 `#!` 行决定解释器，`#!/usr/bin/env bash` 会真正用 bash 执行（`set -o pipefail`、数组、`[[ ]]` 都可用）。可用 `--shell=NAME` 覆盖。支持 `sh`、`bash`、`zsh`、`dash`、`ksh`、`ash`；其他解释器在本地就以 `error_kind: config` 拒绝，不建立连接。最终解释器体现在 `action.script_runner`。 - dry-run/结果暴露 payload SHA-256 与字节数，默认不回传脚本全文。 - 多主机 `--jsonl` 输出 `run_started` / `target_*` / `run_finished`。 diff --git a/docs/zh/apply.md b/docs/zh/apply.md index cfa78c8..e13ca19 100644 --- a/docs/zh/apply.md +++ b/docs/zh/apply.md @@ -52,6 +52,11 @@ POSIX-rename 扩展返回 `SSH_FX_OP_UNSUPPORTED` 时可尝试普通 rename， SFTP 以 SSH 用户身份运行。目标对该用户不可写时使用 `--sudo`。sshx 先把 payload 暂存到远端 home，再通过 stdin 执行特权安装脚本；脚本不会留在主机上。 +写入备份或替换临时文件前，`apply` 会先检查 SSH 用户是否能写入并访问目标父目录。 +原子替换要求父目录同时具备写入和执行权限，与目标文件所有者无关。权限不足时返回 +`error_kind: parent_directory_not_writable`；已配置 sudo 密码 key 时会提示 `--sudo`。 +该预检不能阻止检查后发生的并发权限变更。 + 校验和 reload 用另一次 `sshx run`： ```bash diff --git a/docs/zh/mcp.md b/docs/zh/mcp.md index d5c91c8..5601e10 100644 --- a/docs/zh/mcp.md +++ b/docs/zh/mcp.md @@ -40,6 +40,10 @@ Claude Desktop / 通用 MCP 客户端条目： | `sshx_transfer` | `--transfer` | 经本机中转的服务器到服务器流式传输 | | `sshx_host_list` | `--host-list --json` | 只读 `sshx.hosts.v1` 清单 | +带行过滤条件的 SQL 若需扩大为整表快照，默认会阻断。只有确实接受更宽的 before-image +时，才在 `sshx_sql` 中设置 `allow_full_table_backup: true`；这不允许没有 `WHERE` 的 +`UPDATE` / `DELETE`。 + 工具结果就是 CLI 的版本化 JSON（例如 `sshx_run` 的 `sshx.result.v1`），因此 `success`、`error_kind`、`completion` 和重试指引与 CLI 文档完全一致。子进程 非零退出会把 MCP 结果标成 tool error，但保留结构化载荷。 diff --git a/internal/app/app.go b/internal/app/app.go index 6a60fef..14842eb 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -603,6 +603,7 @@ func resolveHostFromSettings(config *sshclient.Config) error { sudoKey := hostConfig.EffectiveSudoPasswordKey() if sudoKey != "" && !sudoKeyChosen(config) { config.SudoKey = sudoKey + config.SudoKeyConfigured = true logger.GetLogger().Success("Using sudo password key: %s", sudoKey) } // SSH login password key is a distinct role and never falls back to sudo keys. diff --git a/internal/app/apply.go b/internal/app/apply.go index b4af12d..07b0a31 100644 --- a/internal/app/apply.go +++ b/internal/app/apply.go @@ -137,13 +137,14 @@ func HandleApply(config *sshclient.Config, audit *auditRecorder) (err error) { run.phase = "apply" outcome, applyErr := client.ApplyRegularFile(sshclient.ApplyRequest{ - RemotePath: config.RemotePath, - Payload: payload, - ExpectSHA256: config.ApplyExpectSHA256, - Backup: !config.ApplyNoBackup, - BackupDir: config.ApplyBackupDir, - Force: config.Force, - UseSudo: config.ApplyUseSudo, + RemotePath: config.RemotePath, + Payload: payload, + ExpectSHA256: config.ApplyExpectSHA256, + Backup: !config.ApplyNoBackup, + BackupDir: config.ApplyBackupDir, + Force: config.Force, + UseSudo: config.ApplyUseSudo, + SudoConfigured: config.SudoKeyConfigured, }) if applyErr != nil { run.outcome = outcome diff --git a/internal/app/cli_surface_test.go b/internal/app/cli_surface_test.go index 2cce405..6b486d3 100644 --- a/internal/app/cli_surface_test.go +++ b/internal/app/cli_surface_test.go @@ -173,6 +173,17 @@ func TestParseArgsSQLStatementSources(t *testing.T) { require.Contains(t, missing.ArgumentError, "read --statement-file") } +func TestParseArgsSQLTargetAlias(t *testing.T) { + for _, selector := range []string{"--target=db", "--host=db", "-h=db"} { + t.Run(selector, func(t *testing.T) { + config := ParseArgs([]string{"sshx", "sql", selector, "--db=app", "SELECT 1"}) + require.Empty(t, config.ArgumentError) + require.Equal(t, "db", config.Host) + require.Equal(t, "SELECT 1", config.SQLStatement) + }) + } +} + // The suggestion lists must describe options the parser really accepts, so a // typo never points at a name that does not exist. func TestCompatOptionNamesAreRecognized(t *testing.T) { diff --git a/internal/app/config.go b/internal/app/config.go index cb10551..7d715ff 100644 --- a/internal/app/config.go +++ b/internal/app/config.go @@ -71,6 +71,7 @@ func applySudoKeyFlag(config *sshclient.Config, arg string) bool { case strings.HasPrefix(arg, "-pk="), strings.HasPrefix(arg, "--password-key="), strings.HasPrefix(arg, "--sudo-password-key="): config.SudoKey = strings.SplitN(arg, "=", 2)[1] config.SudoKeySet = true + config.SudoKeyConfigured = config.SudoKey != "" return true default: return false @@ -329,6 +330,7 @@ func ParseArgs(args []string) *sshclient.Config { } sudoKey := os.Getenv("SSH_SUDO_KEY") + config.SudoKeyConfigured = sudoKey != "" if sudoKey == "" { sudoKey = sshclient.DefaultSudoKey } @@ -835,12 +837,12 @@ func parseRunArgs(config *sshclient.Config, args []string) { // starts. It feeds the "did you mean" suggestion for an unrecognized option; // TestSQLOptionNamesAreRecognized fails when an entry is not actually parsed. var sqlOptionNames = []string{ - "-h", "--host", "-p", "--port", "-u", "--user", "-i", "--key", "-pk", + "-h", "--host", "--target", "-p", "--port", "-u", "--user", "-i", "--key", "-pk", "--password-key", "--sudo-password-key", "--ssh-password-key", "--no-key", "--password-only", "--key-auth", "--accept-unknown-host", "--insecure-hostkey", "--strict-host-key", "--known-hosts", "--engine", "--db", "--database", "--db-file", "--db-user", "--db-host", "--db-port", "--db-password-key", - "--statement-file", "--row-threshold", "--allow-full-table", "--no-backup", + "--statement-file", "--row-threshold", "--allow-full-table", "--allow-full-table-backup", "--no-backup", "--explain", "--backup-dir", "--docker", "--db-cred-from", "--cred-cache", "--cred-refresh", "--sudo", "--force", "-f", "--dry-run", "--json", "--timeout", "--bind", "--via", "--audit-output", "--no-audit", @@ -926,7 +928,7 @@ func parseSQLArgs(config *sshclient.Config, args []string) { case applyLifecycleFlag(config, arg): case strings.HasPrefix(arg, "--bypass-reason="): config.BypassReason = strings.TrimPrefix(arg, "--bypass-reason=") - case strings.HasPrefix(arg, "-h="), strings.HasPrefix(arg, "--host="): + case strings.HasPrefix(arg, "-h="), strings.HasPrefix(arg, "--host="), strings.HasPrefix(arg, "--target="): config.Host = strings.SplitN(arg, "=", 2)[1] case strings.HasPrefix(arg, "-p="), strings.HasPrefix(arg, "--port="): config.Port = strings.SplitN(arg, "=", 2)[1] @@ -977,6 +979,8 @@ func parseSQLArgs(config *sshclient.Config, args []string) { } case arg == "--allow-full-table": config.SQLAllowFullTable = true + case arg == "--allow-full-table-backup": + config.SQLAllowFullTableBackup = true case arg == "--no-backup": config.SQLNoBackup = true case arg == "--explain": diff --git a/internal/app/dryrun.go b/internal/app/dryrun.go index 6c39b6a..5641979 100644 --- a/internal/app/dryrun.go +++ b/internal/app/dryrun.go @@ -141,23 +141,24 @@ type applyDryRunPlan struct { // The backup decision shown here uses no EXPLAIN estimate; a row-level backup // may still upgrade to a table dump at execution time. type sqlDryRunPlan struct { - Engine string `json:"engine"` - Database string `json:"database"` - Statement string `json:"statement"` - StatementHash string `json:"statement_sha256"` - Class string `json:"class,omitempty"` - Verb string `json:"verb,omitempty"` - Table string `json:"table,omitempty"` - HasWhere bool `json:"has_where"` - Docker string `json:"docker,omitempty"` - CredSource string `json:"cred_source,omitempty"` - CredCache string `json:"cred_cache,omitempty"` - PolicyCheck dryRunStatus `json:"policy_check"` - BackupKind string `json:"backup_kind,omitempty"` - BackupReason string `json:"backup_reason,omitempty"` - ExplainCommand string `json:"explain_command,omitempty"` - ExecuteCommand string `json:"execute_command,omitempty"` - UseSudo bool `json:"use_sudo,omitempty"` + Engine string `json:"engine"` + Database string `json:"database"` + Statement string `json:"statement"` + StatementHash string `json:"statement_sha256"` + Class string `json:"class,omitempty"` + Verb string `json:"verb,omitempty"` + Table string `json:"table,omitempty"` + HasWhere bool `json:"has_where"` + Docker string `json:"docker,omitempty"` + CredSource string `json:"cred_source,omitempty"` + CredCache string `json:"cred_cache,omitempty"` + PolicyCheck dryRunStatus `json:"policy_check"` + BackupKind string `json:"backup_kind,omitempty"` + BackupReasonCode string `json:"backup_reason_code,omitempty"` + BackupReason string `json:"backup_reason,omitempty"` + ExplainCommand string `json:"explain_command,omitempty"` + ExecuteCommand string `json:"execute_command,omitempty"` + UseSudo bool `json:"use_sudo,omitempty"` } func emitDryRunPlan(config *sshclient.Config) error { @@ -861,13 +862,24 @@ func fillDryRunSQL(config *sshclient.Config, plan *dryRunPlan) { return } sqlPlan.BackupKind = string(backup.Kind) + sqlPlan.BackupReasonCode = backup.ReasonCode sqlPlan.BackupReason = backup.Reason - if backup.Kind == sqlsafe.BackupRows { - sqlPlan.BackupReason += " (may upgrade to a full-table CSV snapshot if the EXPLAIN estimate exceeds the row threshold)" + if backup.Kind == sqlsafe.BackupRows && sqlsafe.NormalizeEngine(config.SQLEngine) != sqlsafe.EngineSQLite { + sqlPlan.BackupReason += " (an EXPLAIN estimate above --row-threshold may select a full-table backup, which is blocked by default; pass --allow-full-table-backup to opt in)" } if cls.Class == sqlsafe.ClassDML && !opts.NoBackup && backup.Kind != sqlsafe.BackupFile { sqlPlan.BackupReason += " (runtime catalog preflight blocks triggers, rewrite rules, partitions, and cascading referential actions)" } + if scopeErr := sqlsafe.CheckBackupScope(cls, backup, opts); scopeErr != nil { + kind := "blocked" + if typed, ok := scopeErr.(interface{ ErrorKind() string }); ok { + kind = typed.ErrorKind() + } + plan.SafetyCheck = dryRunStatus{Status: "blocked", ErrorKind: kind, Message: scopeErr.Error()} + sqlPlan.PolicyCheck = plan.SafetyCheck + plan.Valid = false + return + } conn := newSQLExecutor(config, "") if cls.Class == sqlsafe.ClassDML || config.SQLExplainOnly { diff --git a/internal/app/lifecycle.go b/internal/app/lifecycle.go index 6942cee..85c4ccb 100644 --- a/internal/app/lifecycle.go +++ b/internal/app/lifecycle.go @@ -85,7 +85,19 @@ func reportPlanFailure(config *sshclient.Config, audit *auditRecorder, err error if classification, classifyErr := sqlsafe.ClassifyFor(config.SQLEngine, config.SQLStatement); classifyErr == nil { run.cls = classification } + if prepared := preparedFrom(config); prepared != nil && prepared.preview.SQL != nil { + preview := prepared.preview.SQL + if preview.BackupKind != "" && preview.BackupKind != string(sqlsafe.BackupNone) { + run.backup = &sqlBackupJSON{ + Kind: preview.BackupKind, Table: preview.Table, + ReasonCode: preview.BackupReasonCode, Reason: preview.BackupReason, + } + } + } failure := run.baseResult() + if failure.Backup != nil { + failure.Evidence.BackupStatus = "not_performed" + } failure.ExitCode, failure.ErrorKind, failure.Error = -1, kind, redactError(err) value = failure case "inspect": diff --git a/internal/app/mcp.go b/internal/app/mcp.go index ce42a48..6b7320e 100644 --- a/internal/app/mcp.go +++ b/internal/app/mcp.go @@ -717,33 +717,34 @@ type mcpRunInput struct { } type mcpSQLInput struct { - ExpectPlan string `json:"expect_plan,omitempty" jsonschema:"Expected sha256 execution plan hash; mismatch is rejected before connecting."` - HostTimeoutSecs int `json:"host_timeout_seconds,omitempty" jsonschema:"Optional whole-target budget in seconds."` - GlobalTimeoutSecs int `json:"global_timeout_seconds,omitempty" jsonschema:"Optional whole-operation budget in seconds; MCP also has a 30-minute watchdog."` - BypassReason string `json:"bypass_reason,omitempty" jsonschema:"Optional recorded justification for SQL policy bypasses."` - Target string `json:"target" jsonschema:"Configured host name or address to reach over SSH."` - Statement string `json:"statement" jsonschema:"Exactly one SQL statement; multi-statement input is blocked fail-closed."` - Engine string `json:"engine,omitempty" jsonschema:"postgres (default), sqlite, or mysql."` - DB string `json:"db,omitempty" jsonschema:"PostgreSQL database name."` - DBFile string `json:"db_file,omitempty" jsonschema:"Absolute SQLite database file path (required for engine=sqlite)."` - DBUser string `json:"db_user,omitempty" jsonschema:"Database role."` - DBHost string `json:"db_host,omitempty" jsonschema:"Database host as seen from the remote host."` - DBPort string `json:"db_port,omitempty" jsonschema:"Database port."` - DBPasswordKey string `json:"db_password_key,omitempty" jsonschema:"OS-keyring key holding the DB password; delivered via stdin, never argv."` - Docker string `json:"docker,omitempty" jsonschema:"Run the database client inside this container via docker exec -i."` - DBCredFrom string `json:"db_cred_from,omitempty" jsonschema:"Resolve credentials on the remote host: docker: or env-file:."` - CredCache string `json:"cred_cache,omitempty" jsonschema:"off or a duration for caching remotely resolved credentials (default 15m)."` - CredRefresh bool `json:"cred_refresh,omitempty" jsonschema:"Drop the cached credential entry and re-resolve."` - Explain bool `json:"explain,omitempty" jsonschema:"Run EXPLAIN only; never executes the statement."` - RowThreshold int `json:"row_threshold,omitempty" jsonschema:"EXPLAIN row estimate that upgrades a row backup to a full-table snapshot (default 1000)."` - AllowFullTable bool `json:"allow_full_table,omitempty" jsonschema:"Required for UPDATE/DELETE without a WHERE clause."` - NoBackup bool `json:"no_backup,omitempty" jsonschema:"Skip the pre-change backup; requires force."` - BackupDir string `json:"backup_dir,omitempty" jsonschema:"Remote backup directory (default ~/.sshx/sql-backups)."` - Sudo bool `json:"sudo,omitempty" jsonschema:"Run the remote database client via sudo -S when the SSH user cannot open the database file."` - Force bool `json:"force,omitempty" jsonschema:"Confirms DDL; destructive DDL also requires no_backup."` - DryRun bool `json:"dry_run,omitempty" jsonschema:"Preview the guarded SQL plan without connecting."` - TimeoutSecs int `json:"timeout_seconds,omitempty" jsonschema:"Remote execution timeout in seconds."` - Bind string `json:"bind,omitempty" jsonschema:"Local source address: literal IP or network interface name."` + ExpectPlan string `json:"expect_plan,omitempty" jsonschema:"Expected sha256 execution plan hash; mismatch is rejected before connecting."` + HostTimeoutSecs int `json:"host_timeout_seconds,omitempty" jsonschema:"Optional whole-target budget in seconds."` + GlobalTimeoutSecs int `json:"global_timeout_seconds,omitempty" jsonschema:"Optional whole-operation budget in seconds; MCP also has a 30-minute watchdog."` + BypassReason string `json:"bypass_reason,omitempty" jsonschema:"Optional recorded justification for SQL policy bypasses."` + Target string `json:"target" jsonschema:"Configured host name or address to reach over SSH."` + Statement string `json:"statement" jsonschema:"Exactly one SQL statement; multi-statement input is blocked fail-closed."` + Engine string `json:"engine,omitempty" jsonschema:"postgres (default), sqlite, or mysql."` + DB string `json:"db,omitempty" jsonschema:"PostgreSQL database name."` + DBFile string `json:"db_file,omitempty" jsonschema:"Absolute SQLite database file path (required for engine=sqlite)."` + DBUser string `json:"db_user,omitempty" jsonschema:"Database role."` + DBHost string `json:"db_host,omitempty" jsonschema:"Database host as seen from the remote host."` + DBPort string `json:"db_port,omitempty" jsonschema:"Database port."` + DBPasswordKey string `json:"db_password_key,omitempty" jsonschema:"OS-keyring key holding the DB password; delivered via stdin, never argv."` + Docker string `json:"docker,omitempty" jsonschema:"Run the database client inside this container via docker exec -i."` + DBCredFrom string `json:"db_cred_from,omitempty" jsonschema:"Resolve credentials on the remote host: docker: or env-file:."` + CredCache string `json:"cred_cache,omitempty" jsonschema:"off or a duration for caching remotely resolved credentials (default 15m)."` + CredRefresh bool `json:"cred_refresh,omitempty" jsonschema:"Drop the cached credential entry and re-resolve."` + Explain bool `json:"explain,omitempty" jsonschema:"Run EXPLAIN only; never executes the statement."` + RowThreshold int `json:"row_threshold,omitempty" jsonschema:"EXPLAIN row estimate that may widen a row backup to a full-table snapshot (default 1000); expansion is blocked by default."` + AllowFullTable bool `json:"allow_full_table,omitempty" jsonschema:"Required for UPDATE/DELETE without a WHERE clause."` + AllowFullTableBackup bool `json:"allow_full_table_backup,omitempty" jsonschema:"Explicitly permit a full-table before-image when a row-filtered mutation cannot be backed up narrowly; does not permit a mutation without WHERE."` + NoBackup bool `json:"no_backup,omitempty" jsonschema:"Skip the pre-change backup; requires force."` + BackupDir string `json:"backup_dir,omitempty" jsonschema:"Remote backup directory (default ~/.sshx/sql-backups)."` + Sudo bool `json:"sudo,omitempty" jsonschema:"Run the remote database client via sudo -S when the SSH user cannot open the database file."` + Force bool `json:"force,omitempty" jsonschema:"Confirms DDL; destructive DDL also requires no_backup."` + DryRun bool `json:"dry_run,omitempty" jsonschema:"Preview the guarded SQL plan without connecting."` + TimeoutSecs int `json:"timeout_seconds,omitempty" jsonschema:"Remote execution timeout in seconds."` + Bind string `json:"bind,omitempty" jsonschema:"Local source address: literal IP or network interface name."` } type mcpROSInput struct { @@ -986,6 +987,9 @@ func buildSQLArgs(in mcpSQLInput) ([]string, error) { if in.AllowFullTable { args = append(args, "--allow-full-table") } + if in.AllowFullTableBackup { + args = append(args, "--allow-full-table-backup") + } if in.NoBackup { args = append(args, "--no-backup") } diff --git a/internal/app/mcp_test.go b/internal/app/mcp_test.go index 1e1623d..d1a119e 100644 --- a/internal/app/mcp_test.go +++ b/internal/app/mcp_test.go @@ -607,16 +607,17 @@ func TestBuildRunArgsRequiresExactlyOnePayload(t *testing.T) { func TestBuildSQLArgs(t *testing.T) { args, err := buildSQLArgs(mcpSQLInput{ - Target: "db-1", - Statement: "SELECT count(*) FROM users", - Engine: "postgres", - DB: "app", - DBUser: "app", - DBPasswordKey: "app-db", - Explain: true, - RowThreshold: 500, - Sudo: true, - DryRun: true, + Target: "db-1", + Statement: "SELECT count(*) FROM users", + Engine: "postgres", + DB: "app", + DBUser: "app", + DBPasswordKey: "app-db", + Explain: true, + RowThreshold: 500, + AllowFullTableBackup: true, + Sudo: true, + DryRun: true, }) if err != nil { t.Fatalf("buildSQLArgs: %v", err) @@ -625,6 +626,7 @@ func TestBuildSQLArgs(t *testing.T) { "sql", "--json", "-h=db-1", "--engine=postgres", "--db=app", "--db-user=app", "--db-password-key=app-db", "--explain", "--row-threshold=500", + "--allow-full-table-backup", "--sudo", "--dry-run", "--", "SELECT count(*) FROM users", } diff --git a/internal/app/sql.go b/internal/app/sql.go index ccaf898..416ab42 100644 --- a/internal/app/sql.go +++ b/internal/app/sql.go @@ -21,6 +21,7 @@ type sqlBackupJSON struct { Table string `json:"table,omitempty"` Path string `json:"path,omitempty"` Rows *int64 `json:"rows,omitempty"` + ReasonCode string `json:"reason_code,omitempty"` Reason string `json:"reason,omitempty"` RestoreHint string `json:"restore_hint,omitempty"` } @@ -92,10 +93,11 @@ type sqlRun struct { func sqlOptions(config *sshclient.Config) sqlsafe.Options { return sqlsafe.Options{ - Force: config.Force, - AllowFullTable: config.SQLAllowFullTable, - NoBackup: config.SQLNoBackup, - RowThreshold: config.SQLRowThreshold, + Force: config.Force, + AllowFullTable: config.SQLAllowFullTable, + AllowFullTableBackup: config.SQLAllowFullTableBackup, + NoBackup: config.SQLNoBackup, + RowThreshold: config.SQLRowThreshold, } } @@ -505,6 +507,30 @@ func (r *sqlRun) backupPhase() error { if planErr != nil { return r.fail("blocked", planErr) } + r.backup = &sqlBackupJSON{ + Kind: string(plan.Kind), Table: plan.Table, + ReasonCode: plan.ReasonCode, Reason: plan.Reason, + } + if plan.Kind == sqlsafe.BackupNone { + return nil + } + + path := sqlsafe.BackupPath(r.config.SQLBackupDir, r.config.SQLDatabase, plan.Table, plan.Kind) + if sqlsafe.NormalizeEngine(r.config.SQLEngine) == sqlsafe.EngineMySQL { + path = strings.TrimSuffix(path, ".csv") + ".mysql-hex" + } + r.backup.Path = path + r.backup.RestoreHint = sqlsafe.RestoreHintFor(r.config.SQLEngine, plan, path) + if scopeErr := sqlsafe.CheckBackupScope(r.cls, plan, r.opts); scopeErr != nil { + r.evidence.BackupStatus = "not_performed" + kind := "blocked" + var typed interface{ ErrorKind() string } + if errors.As(scopeErr, &typed) { + kind = typed.ErrorKind() + } + return r.fail(kind, scopeErr) + } + needImpact := !r.opts.NoBackup && plan.Kind != sqlsafe.BackupFile && (plan.Kind != sqlsafe.BackupNone || r.cls.Class == sqlsafe.ClassDML) if needImpact { @@ -518,17 +544,6 @@ func (r *sqlRun) backupPhase() error { r.cls.Verb)) } } - r.backup = &sqlBackupJSON{Kind: string(plan.Kind), Table: plan.Table, Reason: plan.Reason} - if plan.Kind == sqlsafe.BackupNone { - return nil - } - - path := sqlsafe.BackupPath(r.config.SQLBackupDir, r.config.SQLDatabase, plan.Table, plan.Kind) - if sqlsafe.NormalizeEngine(r.config.SQLEngine) == sqlsafe.EngineMySQL { - path = strings.TrimSuffix(path, ".csv") + ".mysql-hex" - } - r.backup.Path = path - r.backup.RestoreHint = sqlsafe.RestoreHintFor(r.config.SQLEngine, plan, path) return nil } @@ -851,7 +866,7 @@ func addSQLEvidenceConditions(result *sqlJSONResult) { execution.Condition{Kind: "sql_affected_rows_semantics", Subject: target + ":" + result.Table, Expected: semantics, Observed: evidence.AffectedRowsSemantics, Status: semanticsStatus}, ) } - if result.Backup == nil || result.Backup.Kind == string(sqlsafe.BackupNone) { + if result.Backup == nil || result.Backup.Kind == string(sqlsafe.BackupNone) || result.Backup.Path == "" { return } backupStatus := evidence.BackupStatus diff --git a/internal/app/sql_test.go b/internal/app/sql_test.go index 670b0b2..d99d141 100644 --- a/internal/app/sql_test.go +++ b/internal/app/sql_test.go @@ -73,12 +73,12 @@ func TestParseArgs_SQLAfterDoubleDash(t *testing.T) { func TestParseArgs_SQLSafetyFlags(t *testing.T) { config := ParseArgs([]string{ - "sshx", "sql", "-h=db1", "--db=app", "--allow-full-table", + "sshx", "sql", "-h=db1", "--db=app", "--allow-full-table", "--allow-full-table-backup", "--no-backup", "--force", "--explain", "--row-threshold=50", "--backup-dir=/tmp/bk", "--dry-run", "DELETE FROM users", }) - if !config.SQLAllowFullTable || !config.SQLNoBackup || !config.Force || !config.SQLExplainOnly { + if !config.SQLAllowFullTable || !config.SQLAllowFullTableBackup || !config.SQLNoBackup || !config.Force || !config.SQLExplainOnly { t.Fatalf("safety flags not parsed: %#v", config) } if config.SQLRowThreshold != 50 { @@ -220,6 +220,34 @@ func TestFillDryRunSQL(t *testing.T) { } }) + t.Run("unreproducible row filter blocks full-table backup by default", func(t *testing.T) { + config := ParseArgs([]string{ + "sshx", "sql", "-h=db1", "--db=app", "--dry-run", + "UPDATE users SET active=false WHERE id IN (SELECT id FROM candidates)", + }) + plan := buildDryRunPlan(config) + if plan.SQL == nil || plan.SQL.BackupKind != string(sqlsafe.BackupTable) { + t.Fatalf("expected a visible full-table backup scope, got %#v", plan.SQL) + } + if plan.SafetyCheck.Status != "blocked" || plan.SafetyCheck.ErrorKind != "unreproducible_select" { + t.Fatalf("expected unreproducible_select block, got %#v", plan.SafetyCheck) + } + if !strings.Contains(plan.SafetyCheck.Message, "--allow-full-table-backup") { + t.Fatalf("block must describe the explicit opt-in: %#v", plan.SafetyCheck) + } + if plan.WouldConnect || plan.WouldExecute || plan.WouldMutateRemote { + t.Fatalf("blocked backup plan must not report side effects: %#v", plan) + } + + allowed := buildDryRunPlan(ParseArgs([]string{ + "sshx", "sql", "-h=db1", "--db=app", "--dry-run", "--allow-full-table-backup", + "UPDATE users SET active=false WHERE id IN (SELECT id FROM candidates)", + })) + if !allowed.Valid || allowed.SQL == nil || allowed.SQL.BackupKind != string(sqlsafe.BackupTable) { + t.Fatalf("explicitly allowed full-table backup should remain visible: %#v", allowed) + } + }) + t.Run("full table delete is blocked", func(t *testing.T) { config := ParseArgs([]string{ "sshx", "sql", "-h=db1", "--db=app", "--dry-run", "DELETE FROM users", @@ -278,7 +306,7 @@ func TestFillDryRunSQL(t *testing.T) { if plan.SQL.Engine != sqlsafe.EngineSQLite || plan.SQL.Database != "/var/lib/app/app.db" { t.Fatalf("unexpected sqlite identity: %#v", plan.SQL) } - if plan.SQL.Class != string(sqlsafe.ClassDML) || plan.SQL.BackupKind != string(sqlsafe.BackupTable) { + if plan.SQL.Class != string(sqlsafe.ClassDML) || plan.SQL.BackupKind != string(sqlsafe.BackupRows) { t.Fatalf("unexpected sqlite plan: %#v", plan.SQL) } if !strings.Contains(plan.SQL.ExecuteCommand, "sqlite3 -batch -bail /var/lib/app/app.db") { diff --git a/internal/app/usage_sections.go b/internal/app/usage_sections.go index aac5095..9ae1062 100644 --- a/internal/app/usage_sections.go +++ b/internal/app/usage_sections.go @@ -85,6 +85,11 @@ Run Contract (preferred for Agents): --script-stdin byte-preserving script from stdin --shell=NAME interpreter override: sh, bash, zsh, dash, ksh, ash (default: the script's #! line, else sh) + --sudo run the selected script interpreter via sudo + + --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. Limits / policy: --concurrency=N default 4, hard max 32 @@ -339,12 +344,13 @@ Inspection Capabilities: const usageSQL = ` Guarded SQL Execution: - sshx sql -h= --db= [options] "" + sshx sql --target= --db= [options] "" sshx sql -h= --engine=sqlite --db-file=/abs/path.db [options] "SQL" sshx sql -h= --db= [options] -- sshx sql -h= --db= [options] --statement-file=./query.sql printf '%s' 'select 1' | sshx sql -h= --db= [options] + Host selectors --target=NAME, --host=NAME, and -h=NAME are equivalent. The statement may be a positional argument, everything after --, a local file via --statement-file=PATH, or stdin when it is piped rather than a terminal. Reading stdin waits for EOF, so close stdin when another process holds the pipe @@ -391,8 +397,10 @@ Guarded SQL Execution: Expired entries are deleted from the keyring. --cred-refresh Drop the cached entry and re-resolve from the source --explain Run EXPLAIN only; never executes the statement - --row-threshold=N EXPLAIN row estimate that upgrades a row backup to a - full-table CSV snapshot (default: 1000) + --row-threshold=N EXPLAIN estimate that may widen a row backup to a + full-table snapshot (default: 1000); blocked by default + --allow-full-table-backup Explicitly permit a full-table backup before-image for a + row-filtered mutation when a row snapshot is unavailable --allow-full-table Required for UPDATE/DELETE without a WHERE clause --no-backup Skip pre-change backup (requires --force) --backup-dir=PATH Remote backup directory (default: ~/.sshx/sql-backups) @@ -402,8 +410,9 @@ Guarded SQL Execution: Safety pipeline for data changes: classify locally (fail-closed), gate by policy, then snapshot and execute. PostgreSQL runs EXPLAIN (FORMAT JSON) and snapshots rows or the table under one transaction plus a SHARE ROW - EXCLUSIVE lock. SQLite skips row estimates and snapshots the table (CSV) - or the whole file under BEGIN IMMEDIATE. Whole-file .backup uses a second + EXCLUSIVE lock. SQLite uses a row CSV before-image when its WHERE clause is + stable, otherwise a full-table before-image is blocked unless explicitly + allowed. Whole-file .backup uses a second read-only client while the mutation client holds the writer lock; mutation is sent only after snapshot completion. SELECT and other reads skip EXPLAIN and backups. @@ -479,9 +488,12 @@ Guarded File Apply: sshx apply --target= --path=/abs/remote.conf --from=./local.conf --json Replaces one remote regular file. sshx reads the current file, optionally - checks --expect-sha256, writes an owner-only backup, then atomically replaces - the target while preserving mode and owner. Reload/restart is not part of - this command — run a separate sshx run after apply succeeds. + checks --expect-sha256, checks parent-directory write access before creating + backup/temp files, then atomically replaces the target while preserving mode + and owner. Atomic replacement needs write and execute permission on the + parent directory; error_kind=parent_directory_not_writable reports a denial. + Reload/restart is not part of this command — run a separate sshx run after + apply succeeds. --path=PATH Absolute remote file path (required) --from=PATH Local source file (required) diff --git a/internal/app/usage_test.go b/internal/app/usage_test.go index 5d878d6..c2b2ed2 100644 --- a/internal/app/usage_test.go +++ b/internal/app/usage_test.go @@ -228,6 +228,34 @@ func TestPrintVerbUsageJSON(t *testing.T) { } } +func TestSQLHelpDocumentsTargetHostSelector(t *testing.T) { + output := string(captureStdout(t, func() { + require.NoError(t, PrintVerbUsage(&sshclient.Config{HelpVerb: "sql"})) + })) + require.Contains(t, output, "--target=NAME") + require.Contains(t, output, "Host selectors --target=NAME, --host=NAME, and -h=NAME are equivalent") +} + +func TestRunHelpExplainsSudoScriptPayloads(t *testing.T) { + output := string(captureStdout(t, func() { + require.NoError(t, PrintVerbUsage(&sshclient.Config{HelpVerb: "run"})) + })) + require.Contains(t, output, "--sudo") + require.Contains(t, output, "run the selected script interpreter via sudo") + 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") +} + +func TestSQLHelpDocumentsFullTableBackupOptIn(t *testing.T) { + output := string(captureStdout(t, func() { + require.NoError(t, PrintVerbUsage(&sshclient.Config{HelpVerb: "sql"})) + })) + require.Contains(t, output, "--allow-full-table-backup") + require.Contains(t, output, "full-table backup") + require.Contains(t, output, "blocked by default") +} + func TestPrintUsageAdvertisesHelpSurfaces(t *testing.T) { output := string(captureStdout(t, PrintUsage)) require.Contains(t, output, "sshx --help") diff --git a/internal/sqlsafe/dialect.go b/internal/sqlsafe/dialect.go index 85eaed7..4fd8b56 100644 --- a/internal/sqlsafe/dialect.go +++ b/internal/sqlsafe/dialect.go @@ -45,6 +45,7 @@ func (mysqlDialect) DecideBackup(cls *Classification, estimatedRows int64, opts plan.Reason = strings.ReplaceAll(plan.Reason, "CSV", "hex-row") if estimatedRows < 0 && plan.Kind == BackupRows { plan.Kind = BackupTable + plan.ReasonCode = BackupReasonEstimateUnavailable plan.Reason = "MySQL row estimate unavailable; taking a full-table hex-row snapshot" } } diff --git a/internal/sqlsafe/policy.go b/internal/sqlsafe/policy.go index f57854f..71b38dc 100644 --- a/internal/sqlsafe/policy.go +++ b/internal/sqlsafe/policy.go @@ -11,6 +11,9 @@ type Options struct { Force bool // AllowFullTable allows UPDATE/DELETE without a top-level WHERE clause. AllowFullTable bool + // AllowFullTableBackup explicitly permits a full-table before-image for a + // row-filtered mutation when a narrower row snapshot is unavailable. + AllowFullTableBackup bool // NoBackup disables pre-change backups. It requires Force. NoBackup bool // RowThreshold is the estimated-row boundary between a row-level CSV @@ -78,9 +81,45 @@ const ( // BackupPlan describes the pre-change backup decided for one statement. type BackupPlan struct { - Kind BackupKind `json:"kind"` - Table string `json:"table,omitempty"` - Reason string `json:"reason"` + Kind BackupKind `json:"kind"` + Table string `json:"table,omitempty"` + ReasonCode string `json:"reason_code,omitempty"` + Reason string `json:"reason"` +} + +const ( + BackupReasonUnreproducibleSelect = "unreproducible_select" + BackupReasonRowThresholdExceeded = "row_threshold_exceeded" + BackupReasonEstimateUnavailable = "estimate_unavailable" +) + +type backupScopeError struct { + kind string + verb string + reason string +} + +func (e *backupScopeError) Error() string { + return fmt.Sprintf( + "row-filtered %s would require a full-table before-image (%s), which may persist unrelated rows and sensitive columns; pass --allow-full-table-backup to explicitly permit this backup scope", + e.verb, e.reason, + ) +} + +func (e *backupScopeError) ErrorKind() string { return e.kind } + +// CheckBackupScope requires explicit intent before a row-filtered mutation +// writes a whole-table before-image. +func CheckBackupScope(cls *Classification, plan BackupPlan, opts Options) error { + if opts.NoBackup || opts.AllowFullTableBackup || cls.Class != ClassDML || + !cls.HasWhere || plan.Kind != BackupTable { + return nil + } + kind := "full_table_backup_requires_opt_in" + if plan.ReasonCode == BackupReasonUnreproducibleSelect { + kind = BackupReasonUnreproducibleSelect + } + return &backupScopeError{kind: kind, verb: cls.Verb, reason: plan.Reason} } // DecideBackup chooses the backup strategy for a classified statement given @@ -111,7 +150,8 @@ func DecideBackup(cls *Classification, estimatedRows int64, opts Options) (Backu if cls.Destructive || cls.ComplexSource { return BackupPlan{Kind: BackupTable, Table: cls.Table, - Reason: "affected rows cannot be reproduced by a simple SELECT; taking a full-table CSV snapshot"}, nil + ReasonCode: BackupReasonUnreproducibleSelect, + Reason: "affected rows cannot be reproduced by a simple SELECT; taking a full-table CSV snapshot"}, nil } if !cls.HasWhere { return BackupPlan{Kind: BackupTable, Table: cls.Table, @@ -119,15 +159,18 @@ func DecideBackup(cls *Classification, estimatedRows int64, opts Options) (Backu } if containsNewline(cls.WhereClause) { return BackupPlan{Kind: BackupTable, Table: cls.Table, - Reason: "WHERE clause spans multiple lines; taking a full-table CSV snapshot instead of a row snapshot"}, nil + ReasonCode: BackupReasonUnreproducibleSelect, + Reason: "WHERE clause spans multiple lines; taking a full-table CSV snapshot instead of a row snapshot"}, nil } if !stableBackupPredicate(cls.WhereClause) { return BackupPlan{Kind: BackupTable, Table: cls.Table, - Reason: "predicate may be evaluated differently during backup and mutation; taking a full-table snapshot"}, nil + ReasonCode: BackupReasonUnreproducibleSelect, + Reason: "predicate may be evaluated differently during backup and mutation; taking a full-table snapshot"}, nil } if estimatedRows >= 0 && estimatedRows > opts.rowThreshold() { return BackupPlan{Kind: BackupTable, Table: cls.Table, - Reason: fmt.Sprintf("estimated %d affected rows exceeds the row-backup threshold (%d); taking a full-table CSV snapshot", estimatedRows, opts.rowThreshold())}, nil + ReasonCode: BackupReasonRowThresholdExceeded, + Reason: fmt.Sprintf("estimated %d affected rows exceeds the row-backup threshold (%d); taking a full-table CSV snapshot", estimatedRows, opts.rowThreshold())}, nil } return BackupPlan{Kind: BackupRows, Table: cls.Table, Reason: "snapshotting affected rows selected by the statement's WHERE clause"}, nil diff --git a/internal/sqlsafe/sqlite.go b/internal/sqlsafe/sqlite.go index ce5c762..24a64db 100644 --- a/internal/sqlsafe/sqlite.go +++ b/internal/sqlsafe/sqlite.go @@ -132,9 +132,12 @@ func (c SQLiteConn) ExecuteReadCommand(stmt string) RemoteCommand { // ExecuteWithBackupCommand locks the database with BEGIN IMMEDIATE, captures // a preimage before sending the mutation, and acknowledges only after COMMIT. func (c SQLiteConn) ExecuteWithBackupCommand(stmt, table, where, path string, kind BackupKind) (RemoteCommand, error) { - if kind != BackupTable && kind != BackupFile { + if kind != BackupRows && kind != BackupTable && kind != BackupFile { return RemoteCommand{}, fmt.Errorf("unsupported sqlite backup kind %q", kind) } + if kind == BackupRows && (containsNewline(where) || !stableBackupPredicate(where)) { + return RemoteCommand{}, &BlockedError{Reason: "SQLite row backup requires a stable, single-line WHERE clause"} + } if !safeArgRE.MatchString(path) { return RemoteCommand{}, &BlockedError{Reason: "backup path contains characters that cannot be embedded in a sqlite3 script"} } @@ -147,12 +150,15 @@ func (c SQLiteConn) ExecuteWithBackupCommand(stmt, table, where, path string, ki p.BackupForm = "csv" lines := []string{p.sqlite("start", "1"), "BEGIN IMMEDIATE;"} switch kind { - case BackupTable: + case BackupRows, BackupTable: if err := ValidateTableIdent(table); err != nil { return RemoteCommand{}, err } - // A table backup must contain the entire table, not merely the rows - // selected by the mutation. Recheck impact after acquiring the lock. + filter := "" + if kind == BackupRows { + filter = " WHERE " + strings.TrimSpace(where) + } + snapshot := "SELECT * FROM " + table + filter + ";" guard := "__sshx_guard_" + p.Token expression := sqliteRelatedEffectsExpression(table, mutationVerb(stmt)) literal := "'" + strings.ReplaceAll(tableIdentName(table), "'", "''") + "'" @@ -165,7 +171,7 @@ func (c SQLiteConn) ExecuteWithBackupCommand(stmt, table, where, path string, ki p.sqlite("copy", "begin"), ".headers on", ".mode csv", - "SELECT * FROM "+table+";", + snapshot, p.sqlite("copy", "end"), ) case BackupFile: @@ -238,9 +244,9 @@ func tableIdentName(table string) string { return strings.Trim(table, `"`) } -// DecideSQLiteBackup chooses a SQLite backup. L1 never uses row-level CSV: -// bounded single-table DML snapshots the table; everything else that needs a -// backup snapshots the whole database file (always available). +// DecideSQLiteBackup chooses a SQLite backup. Stable row filters use a +// transaction-locked row snapshot; wider or unreproducible changes require a +// table or whole-file snapshot. func DecideSQLiteBackup(cls *Classification, opts Options) (BackupPlan, error) { if opts.NoBackup { return BackupPlan{Kind: BackupNone, Reason: "backups disabled by --no-backup"}, nil @@ -253,9 +259,14 @@ func DecideSQLiteBackup(cls *Classification, opts Options) (BackupPlan, error) { case cls.Class == ClassDDL && !cls.Destructive: return BackupPlan{Kind: BackupNone, Reason: "non-destructive DDL/maintenance statement"}, nil } - if cls.Class == ClassDML && cls.HasWhere && !cls.ComplexSource && cls.Table != "" { + if cls.Class == ClassDML && cls.HasWhere && cls.Table != "" { + if !cls.ComplexSource && !containsNewline(cls.WhereClause) && stableBackupPredicate(cls.WhereClause) { + return BackupPlan{Kind: BackupRows, Table: cls.Table, + Reason: "snapshotting affected rows selected by the statement's stable WHERE clause"}, nil + } return BackupPlan{Kind: BackupTable, Table: cls.Table, - Reason: "SQLite snapshots the whole target table before mutation (no row-estimate gate)"}, nil + ReasonCode: BackupReasonUnreproducibleSelect, + Reason: "affected rows cannot be safely reproduced as a narrow SQLite snapshot; taking a full-table CSV snapshot"}, nil } return BackupPlan{Kind: BackupFile, Table: cls.Table, Reason: "taking a consistent whole-file snapshot with sqlite3 .backup under BEGIN IMMEDIATE"}, nil @@ -275,8 +286,8 @@ func RestoreHintFor(engine string, plan BackupPlan, path string) string { func sqliteRestoreHint(plan BackupPlan, path string) string { switch plan.Kind { - case BackupTable: - return fmt.Sprintf("restore table data with: sqlite3 \".mode csv\" \".import %s %s\" (reconcile existing rows first)", path, plan.Table) + case BackupRows, BackupTable: + return fmt.Sprintf("restore selected table rows with: sqlite3 \".mode csv\" \".import %s %s\" (reconcile existing rows first)", path, plan.Table) case BackupFile: return fmt.Sprintf("restore the database file from %s (stop writers, replace the live file plus any leftover -wal/-shm, then reopen)", path) default: diff --git a/internal/sqlsafe/sqlite_test.go b/internal/sqlsafe/sqlite_test.go index 465d2e4..b721fcb 100644 --- a/internal/sqlsafe/sqlite_test.go +++ b/internal/sqlsafe/sqlite_test.go @@ -123,14 +123,22 @@ func TestValidateSQLitePath(t *testing.T) { } func TestDecideSQLiteBackup(t *testing.T) { - t.Run("bounded update uses table snapshot", func(t *testing.T) { + t.Run("bounded update uses row snapshot", func(t *testing.T) { cls, err := ClassifySQLite("UPDATE users SET x=1 WHERE id=1") require.NoError(t, err) plan, err := DecideSQLiteBackup(cls, Options{}) require.NoError(t, err) - assert.Equal(t, BackupTable, plan.Kind) + assert.Equal(t, BackupRows, plan.Kind) assert.Equal(t, "users", plan.Table) }) + t.Run("unreproducible row filter uses table snapshot", func(t *testing.T) { + cls, err := ClassifySQLite("UPDATE users SET x=1 WHERE id IN (SELECT id FROM candidates)") + require.NoError(t, err) + plan, err := DecideSQLiteBackup(cls, Options{}) + require.NoError(t, err) + assert.Equal(t, BackupTable, plan.Kind) + assert.Equal(t, BackupReasonUnreproducibleSelect, plan.ReasonCode) + }) t.Run("replace uses file snapshot", func(t *testing.T) { cls, err := ClassifySQLite("REPLACE INTO users (id) VALUES (1)") require.NoError(t, err) diff --git a/internal/sqlsafe/sqlsafe_test.go b/internal/sqlsafe/sqlsafe_test.go index 93d7ff5..c738a6f 100644 --- a/internal/sqlsafe/sqlsafe_test.go +++ b/internal/sqlsafe/sqlsafe_test.go @@ -320,9 +320,17 @@ func TestDecideBackup(t *testing.T) { assert.Equal(t, "t", plan.Table) }) t.Run("large update table backup", func(t *testing.T) { - plan, err := DecideBackup(classify(t, "UPDATE t SET x=1 WHERE id > 0"), 50000, Options{}) + cls := classify(t, "UPDATE t SET x=1 WHERE id > 0") + plan, err := DecideBackup(cls, 50000, Options{}) require.NoError(t, err) assert.Equal(t, BackupTable, plan.Kind) + assert.Equal(t, BackupReasonRowThresholdExceeded, plan.ReasonCode) + scopeErr := CheckBackupScope(cls, plan, Options{}) + require.Error(t, scopeErr) + var typed interface{ ErrorKind() string } + require.ErrorAs(t, scopeErr, &typed) + assert.Equal(t, "full_table_backup_requires_opt_in", typed.ErrorKind()) + assert.NoError(t, CheckBackupScope(cls, plan, Options{AllowFullTableBackup: true})) }) t.Run("threshold boundary stays rows", func(t *testing.T) { plan, err := DecideBackup(classify(t, "UPDATE t SET x=1 WHERE id > 0"), DefaultRowThreshold, Options{}) diff --git a/internal/sshclient/apply.go b/internal/sshclient/apply.go index 762e474..d91143f 100644 --- a/internal/sshclient/apply.go +++ b/internal/sshclient/apply.go @@ -40,13 +40,14 @@ var ( // ApplyRequest is one guarded regular-file replacement. type ApplyRequest struct { - RemotePath string - Payload []byte - ExpectSHA256 string - Backup bool - BackupDir string - Force bool - UseSudo bool + RemotePath string + Payload []byte + ExpectSHA256 string + Backup bool + BackupDir string + Force bool + UseSudo bool + SudoConfigured bool } // ApplyOutcome is the observed result of one apply. @@ -103,6 +104,26 @@ type applyCleanupError struct { err error } +type applyParentDirectoryError struct { + path string + sudoConfigured bool +} + +func (e *applyParentDirectoryError) Error() string { + message := fmt.Sprintf( + "parent directory %q is not writable; atomic replacement requires write and execute permission on the parent directory", + e.path, + ) + if e.sudoConfigured { + message += "; a sudo credential is configured, retry with --sudo" + } + return message +} + +func (e *applyParentDirectoryError) ErrorKind() string { + return "parent_directory_not_writable" +} + func (e *applyCleanupError) Error() string { return fmt.Sprintf("cleanup owned apply artifact %s: %v", e.path, e.err) } @@ -219,7 +240,7 @@ func (c *SSHClient) applyWithSFTP(req ApplyRequest) (outcome *ApplyOutcome, err return outcome, clientErr } defer func() { _ = client.Close() }() //nolint:errcheck // teardown cannot invalidate verified remote evidence - outcome, err = c.applySFTPFile(client, req) + outcome, err = c.applySFTPFile(client, req, c.checkApplyParentWritable) if err != nil { var cleanup *applyCleanupError if errors.As(err, &cleanup) { @@ -234,7 +255,7 @@ func (c *SSHClient) applyWithSFTP(req ApplyRequest) (outcome *ApplyOutcome, err return outcome, err } -func (c *SSHClient) applySFTPFile(client *sftp.Client, req ApplyRequest) (outcome *ApplyOutcome, err error) { +func (c *SSHClient) applySFTPFile(client *sftp.Client, req ApplyRequest, checkParentWritable func(string, bool) error) (outcome *ApplyOutcome, err error) { outcome = newApplyOutcome(req) info, statErr := client.Lstat(req.RemotePath) created := false @@ -287,6 +308,20 @@ func (c *SSHClient) applySFTPFile(client *sftp.Client, req ApplyRequest) (outcom return outcome, nil } + parentDir := path.Dir(req.RemotePath) + parentInfo, parentErr := client.Stat(parentDir) + if parentErr != nil { + return outcome, fmt.Errorf("inspect apply parent directory: %w", parentErr) + } + if !parentInfo.IsDir() { + return outcome, fmt.Errorf("%w: parent path %q is not a directory", ErrApplyBlocked, parentDir) + } + if checkParentWritable != nil { + if checkErr := checkParentWritable(parentDir, req.SudoConfigured); checkErr != nil { + return outcome, checkErr + } + } + backupPath := "" if req.Backup && !created { backupPath, err = writeApplyBackup(c, client, req, path.Base(req.RemotePath), before, beforeHash) @@ -348,6 +383,22 @@ func (c *SSHClient) applySFTPFile(client *sftp.Client, req ApplyRequest) (outcom return outcome, nil } +func (c *SSHClient) checkApplyParentWritable(parentDir string, sudoConfigured bool) error { + quotedParent := applyShellQuote(parentDir) + result, err := c.RunCommandWithInput("test -w "+quotedParent+" && test -x "+quotedParent, nil) + if err != nil { + return fmt.Errorf("check apply parent directory access: %w", err) + } + if result.ExitCode != 0 { + return &applyParentDirectoryError{path: parentDir, sudoConfigured: sudoConfigured} + } + return nil +} + +func applyShellQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" +} + func checkApplyPrecondition(created bool, beforeHash string, req ApplyRequest) error { if req.Force { return nil diff --git a/internal/sshclient/apply_fault_test.go b/internal/sshclient/apply_fault_test.go index b9998b0..f7d91ef 100644 --- a/internal/sshclient/apply_fault_test.go +++ b/internal/sshclient/apply_fault_test.go @@ -140,7 +140,7 @@ func TestApplySFTPPartialEvidence(t *testing.T) { if tc.fault == "recheck" { req.ExpectSHA256 = SHA256Hex(before) } - outcome, err := (&SSHClient{}).applySFTPFile(client, req) + outcome, err := (&SSHClient{}).applySFTPFile(client, req, nil) require.Error(t, err) require.NotNil(t, outcome) require.Equal(t, tc.verificationError, errors.Is(err, ErrApplyVerification), "%v", err) @@ -184,12 +184,12 @@ func TestApplySFTPPartialEvidence(t *testing.T) { func TestApplySFTPZeroByteNoopAndPrecondition(t *testing.T) { client, _ := newApplyTestSFTP(t, "") req := ApplyRequest{RemotePath: "/app.conf", Payload: []byte{}, Backup: true, BackupDir: "/backups"} - outcome, err := (&SSHClient{}).applySFTPFile(client, req) + outcome, err := (&SSHClient{}).applySFTPFile(client, req, nil) require.NoError(t, err) require.True(t, outcome.Created) require.True(t, outcome.Verified) require.Equal(t, SHA256Hex(nil), outcome.AfterSHA256) - outcome, err = (&SSHClient{}).applySFTPFile(client, req) + outcome, err = (&SSHClient{}).applySFTPFile(client, req, nil) require.NoError(t, err) require.False(t, *outcome.Executed) require.False(t, outcome.Changed) @@ -197,12 +197,12 @@ func TestApplySFTPZeroByteNoopAndPrecondition(t *testing.T) { require.Empty(t, outcome.BackupPath) req.Payload = []byte("different") req.ExpectSHA256 = SHA256Hex([]byte("wrong")) - outcome, err = (&SSHClient{}).applySFTPFile(client, req) + outcome, err = (&SSHClient{}).applySFTPFile(client, req, nil) require.ErrorIs(t, err, ErrPrecondition) require.Equal(t, SHA256Hex(nil), outcome.BeforeSHA256) require.False(t, *outcome.Executed) req.Force = true - outcome, err = (&SSHClient{}).applySFTPFile(client, req) + outcome, err = (&SSHClient{}).applySFTPFile(client, req, nil) require.NoError(t, err) require.True(t, outcome.Changed) require.True(t, outcome.BackupVerified) @@ -211,7 +211,7 @@ func TestApplySFTPZeroByteNoopAndPrecondition(t *testing.T) { func TestApplyRenameFallbackOnlyWhenUnsupported(t *testing.T) { client, fs := newApplyTestSFTP(t, "unsupported") - outcome, err := (&SSHClient{}).applySFTPFile(client, ApplyRequest{RemotePath: "/app.conf", Payload: []byte("new")}) + outcome, err := (&SSHClient{}).applySFTPFile(client, ApplyRequest{RemotePath: "/app.conf", Payload: []byte("new")}, nil) require.NoError(t, err) require.True(t, outcome.Verified) fs.mu.Lock() diff --git a/internal/sshclient/client.go b/internal/sshclient/client.go index 5c85a2b..a559a45 100644 --- a/internal/sshclient/client.go +++ b/internal/sshclient/client.go @@ -59,6 +59,7 @@ type Config struct { KeyPath string UseKeyAuth bool SudoKey string + SudoKeyConfigured bool // SudoKeySet is true when -pk/--password-key/--sudo-password-key was // present on the command line, including an explicit empty value. // Host inventory must persist the key only when this is set; the @@ -204,6 +205,9 @@ type Config struct { SQLRowThreshold int64 // SQLAllowFullTable permits UPDATE/DELETE without a top-level WHERE. SQLAllowFullTable bool + // SQLAllowFullTableBackup permits whole-table before-images for a + // row-filtered mutation when sshx cannot safely reproduce the selected rows. + SQLAllowFullTableBackup bool // SQLNoBackup skips pre-change backups; requires Force. SQLNoBackup bool // SQLExplainOnly stops after the remote EXPLAIN gate. diff --git a/tests/e2e/apply_e2e_test.go b/tests/e2e/apply_e2e_test.go index 7e508c4..35cf9e3 100644 --- a/tests/e2e/apply_e2e_test.go +++ b/tests/e2e/apply_e2e_test.go @@ -24,6 +24,7 @@ type applyResult struct { PayloadBytes int `json:"payload_bytes"` Completion string `json:"completion"` ErrorKind string `json:"error_kind"` + Error string `json:"error"` RemotePath string `json:"remote_path"` BeforeSHA256 string `json:"before_sha256"` AfterSHA256 string `json:"after_sha256"` @@ -205,6 +206,62 @@ func TestApplyRejectsSymlinkAndReadOnlyTarget(t *testing.T) { assert.ErrorIs(t, err, os.ErrNotExist) } +func TestApplyDiagnosesUnwritableParentBeforeCreatingArtifacts(t *testing.T) { + server := startSSHServer(t, serverOptions{}) + home := t.TempDir() + dir := filepath.Join(server.root, "readonly") + require.NoError(t, os.Mkdir(dir, 0o700)) + target := filepath.Join(dir, "app.conf") + require.NoError(t, os.WriteFile(target, []byte("before\n"), 0o600)) + require.NoError(t, os.Chmod(dir, 0o500)) // #nosec G302 -- make the fixture directory read/execute-only to simulate denied writes. + t.Cleanup(func() { + if err := os.Chmod(dir, 0o700); err != nil { // #nosec G302 -- restore the private fixture directory so t.TempDir can clean up. + t.Errorf("restore apply fixture directory permissions: %v", err) + } + }) + + local := filepath.Join(home, "app.conf") + require.NoError(t, os.WriteFile(local, []byte("after\n"), 0o600)) + backupDir := filepath.Join(server.root, "apply-backups") + args := []string{ + "apply", + "-h=" + server.host, + "-p=" + server.port, + "-u=operator", + "--no-key", + "--json", + "--accept-unknown-host", + "--path=" + filepath.ToSlash(target), + "--from=" + local, + "--backup-dir=" + filepath.ToSlash(backupDir), + } + env := map[string]string{"SSH_PASSWORD": operatorPassword} + + withoutSudo := runSSHX(t, home, args, env) + require.Equal(t, 255, withoutSudo.exitCode, withoutSudo.stderr) + var result applyResult + require.NoError(t, json.Unmarshal([]byte(withoutSudo.stdout), &result)) + assert.Equal(t, "parent_directory_not_writable", result.ErrorKind) + assert.Contains(t, result.Error, "atomic replacement requires write and execute permission on the parent directory") + assert.NotContains(t, result.Error, "--sudo") + assert.Equal(t, "not_started", result.Completion) + got, err := os.ReadFile(target) // #nosec G304 -- target is confined to the isolated fixture directory. + require.NoError(t, err) + assert.Equal(t, "before\n", string(got)) + entries, err := os.ReadDir(dir) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "app.conf", entries[0].Name()) + _, err = os.Stat(backupDir) + assert.ErrorIs(t, err, os.ErrNotExist) + + withConfiguredSudo := runSSHX(t, home, append(append([]string{}, args...), "-pk=apply-sudo"), env) + require.Equal(t, 255, withConfiguredSudo.exitCode, withConfiguredSudo.stderr) + require.NoError(t, json.Unmarshal([]byte(withConfiguredSudo.stdout), &result)) + assert.Equal(t, "parent_directory_not_writable", result.ErrorKind) + assert.Contains(t, result.Error, "--sudo") +} + func TestApplySudoInstallsStagedPayload(t *testing.T) { server := startSSHServer(t, serverOptions{}) home := t.TempDir() diff --git a/tests/e2e/harness_test.go b/tests/e2e/harness_test.go index 366c5c1..2b68cd4 100644 --- a/tests/e2e/harness_test.go +++ b/tests/e2e/harness_test.go @@ -380,6 +380,13 @@ func handleSSHSession(channel ssh.Channel, requests <-chan *ssh.Request, server } server.writeState(strings.TrimPrefix(payload.Command, "set-state ")) _, _ = io.WriteString(channel, "state-updated\n") //nolint:errcheck // fixture response + case strings.HasPrefix(payload.Command, "test -w "): + parent, ok := applyParentAccessCheckPath(payload.Command) + if !ok { + exitCode = 127 + } else if !applyParentDirectoryWritable(parent) { + exitCode = 1 + } case strings.Contains(payload.Command, "sqlite3"): handleSQLiteSession(channel, server, payload.Command) return @@ -410,6 +417,43 @@ func handleSSHSession(channel ssh.Channel, requests <-chan *ssh.Request, server } } +func applyParentAccessCheckPath(command string) (string, bool) { + const ( + writePrefix = "test -w " + executeMark = " && test -x " + ) + if !strings.HasPrefix(command, writePrefix) { + return "", false + } + writeArg, executeArg, ok := strings.Cut(strings.TrimPrefix(command, writePrefix), executeMark) + if !ok { + return "", false + } + writePath, ok := parseShellQuotedPath(writeArg) + if !ok { + return "", false + } + executePath, ok := parseShellQuotedPath(executeArg) + if !ok || executePath != writePath { + return "", false + } + return writePath, true +} + +func parseShellQuotedPath(value string) (string, bool) { + if len(value) < 2 || value[0] != '\'' || value[len(value)-1] != '\'' { + return "", false + } + path := strings.ReplaceAll(value[1:len(value)-1], `'\''`, "'") + quoted := "'" + strings.ReplaceAll(path, "'", `'\''`) + "'" + return path, quoted == value +} + +func applyParentDirectoryWritable(parent string) bool { + info, err := os.Stat(parent) + return err == nil && info.IsDir() && info.Mode().Perm()&0o300 == 0o300 +} + func handleSQLiteSession(channel ssh.Channel, server *testSSHServer, cmdline string) { stdin, err := io.ReadAll(io.LimitReader(channel, 12<<20)) if err != nil { diff --git a/tests/e2e/sql_mysql_e2e_test.go b/tests/e2e/sql_mysql_e2e_test.go index 6801b66..b39730b 100644 --- a/tests/e2e/sql_mysql_e2e_test.go +++ b/tests/e2e/sql_mysql_e2e_test.go @@ -191,7 +191,7 @@ func TestSQLMySQLFakeProtocolUnavailableEstimate(t *testing.T) { }{ {"insert", "INSERT INTO users (id, name) VALUES (2, 'inserted')", "none", nil}, {"insert bypass", "INSERT INTO users (id, name) VALUES (2, 'inserted')", "none", []string{"--force", "--no-backup"}}, - {"update backup", "UPDATE users SET name='new' WHERE id=1", "table", nil}, + {"update backup", "UPDATE users SET name='new' WHERE id=1", "table", []string{"--allow-full-table-backup"}}, } { t.Run(tc.name, func(t *testing.T) { server := startSSHServer(t, serverOptions{}) @@ -216,6 +216,33 @@ func TestSQLMySQLFakeProtocolUnavailableEstimate(t *testing.T) { } } +func TestSQLMySQLFullTableBackupRequiresOptIn(t *testing.T) { + server := startSSHServer(t, serverOptions{}) + installFakeMySQL(t, server) + require.NoError(t, os.WriteFile(filepath.Join(server.root, "mysql-fixture-options.json"), []byte(`{"omit_row_estimate":true}`), 0o600)) + home := t.TempDir() + backupDir := filepath.Join(home, "sql-backups") + args := []string{ + "sql", "-h=" + server.host, "-p=" + server.port, "-u=operator", + "--no-key", "--accept-unknown-host", "--engine=mysql", "--db=app", "--json", + "--backup-dir=" + filepath.ToSlash(backupDir), + "UPDATE users SET name='new' WHERE id=1", + } + result := runSSHX(t, home, args, map[string]string{"SSH_PASSWORD": operatorPassword}) + require.Equal(t, 255, result.exitCode, result.stdout+result.stderr) + var payload sqlResult + require.NoError(t, json.Unmarshal([]byte(result.stdout), &payload)) + assert.False(t, payload.Success) + assert.Equal(t, "full_table_backup_requires_opt_in", payload.ErrorKind) + assert.Contains(t, payload.Error, "--allow-full-table-backup") + require.NotNil(t, payload.Backup) + assert.Equal(t, "table", payload.Backup.Kind) + assert.Equal(t, "estimate_unavailable", payload.Backup.ReasonCode) + assert.Equal(t, "not_performed", payload.Evidence.BackupStatus) + _, err := os.Stat(backupDir) + assert.ErrorIs(t, err, os.ErrNotExist) +} + func TestSQLRealMySQLInsertUnavailableEstimate(t *testing.T) { if testing.Short() || os.Getenv("SSHX_E2E_REAL_SQL") != "1" { t.Skip("requires SSHX_E2E_REAL_SQL=1 and the disposable real MySQL fixture") diff --git a/tests/e2e/sql_sqlite_e2e_test.go b/tests/e2e/sql_sqlite_e2e_test.go index 33dc1ed..7335850 100644 --- a/tests/e2e/sql_sqlite_e2e_test.go +++ b/tests/e2e/sql_sqlite_e2e_test.go @@ -20,6 +20,7 @@ type sqlResult struct { Success bool `json:"success"` ExitCode int `json:"exit_code"` ErrorKind string `json:"error_kind"` + Error string `json:"error"` Class string `json:"class"` Verb string `json:"verb"` Table string `json:"table"` @@ -32,6 +33,8 @@ type sqlResult struct { Backup *struct { Kind string `json:"kind"` Path string `json:"path"` + ReasonCode string `json:"reason_code"` + Reason string `json:"reason"` RestoreHint string `json:"restore_hint"` } `json:"backup"` } @@ -79,7 +82,7 @@ func TestSQLSQLiteDryRunDoesNotConnect(t *testing.T) { require.Equal(t, 0, result.exitCode, result.stderr) assert.Equal(t, connectionsBefore, server.connections.Load()) assert.Contains(t, result.stdout, `"engine": "sqlite"`) - assert.Contains(t, result.stdout, `"backup_kind": "table"`) + assert.Contains(t, result.stdout, `"backup_kind": "rows"`) assert.Contains(t, result.stdout, "sqlite3") } @@ -139,7 +142,7 @@ func TestSQLSQLiteReadAndGuardedUpdate(t *testing.T) { Kind: "sql_affected_rows_semantics", Subject: "sqlite:" + dbPath + ":users", Expected: "sqlite_changes", Observed: "sqlite_changes", Status: "passed", }) require.NotNil(t, updatePayload.Backup) - assert.Equal(t, "table", updatePayload.Backup.Kind) + assert.Equal(t, "rows", updatePayload.Backup.Kind) assert.NotEmpty(t, updatePayload.Backup.Path) assert.Contains(t, updatePayload.Preconditions, execution.Condition{ Kind: "sql_backup", Subject: updatePayload.Backup.Path, Expected: "ready", Observed: "ready", Status: "passed", @@ -158,6 +161,112 @@ func TestSQLSQLiteReadAndGuardedUpdate(t *testing.T) { assert.Equal(t, "new\n", string(got)) } +func TestSQLSQLiteNarrowBackupAndFullTableBackupOptIn(t *testing.T) { + if _, err := exec.LookPath("sqlite3"); err != nil { + t.Skip("requires the real sqlite3 client") + } + server := startSSHServer(t, serverOptions{}) + dbPath := filepath.Join(server.root, "sensitive.db") + setup := exec.Command("sqlite3", dbPath, "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT, api_key TEXT); INSERT INTO users VALUES (1, 'first', 'secret-row-one'), (2, 'second', 'secret-row-two');") // #nosec G204 -- isolated sensitive-column fixture + require.NoError(t, setup.Run()) + + home := t.TempDir() + base := []string{ + "sql", + "-h=" + server.host, + "-p=" + server.port, + "-u=operator", + "--no-key", + "--accept-unknown-host", + "--engine=sqlite", + "--db-file=" + dbPath, + "--json", + } + env := map[string]string{"SSH_PASSWORD": operatorPassword} + rowBackupDir := filepath.Join(server.root, "row-backups") + rowUpdate := runSSHX(t, home, append(append([]string{}, base...), + "--backup-dir="+filepath.ToSlash(rowBackupDir), + "UPDATE users SET name='updated' WHERE id=1", + ), env) + require.Equal(t, 0, rowUpdate.exitCode, rowUpdate.stderr+"\n"+rowUpdate.stdout) + var rowResult sqlResult + require.NoError(t, json.Unmarshal([]byte(rowUpdate.stdout), &rowResult)) + require.NotNil(t, rowResult.Backup) + assert.Equal(t, "rows", rowResult.Backup.Kind) + rowBackup, err := os.ReadFile(rowResult.Backup.Path) // #nosec G304 -- fixture backup is under the isolated SSH root + require.NoError(t, err) + assert.Contains(t, string(rowBackup), "secret-row-one") + assert.NotContains(t, string(rowBackup), "secret-row-two") + + fullBackupDir := filepath.Join(server.root, "full-backups") + updateWithUnreproduciblePredicate := "UPDATE users SET name='blocked' WHERE id IN (SELECT id FROM users WHERE id=2)" + connectionsBefore := server.connections.Load() + preview := runSSHX(t, home, append(append([]string{}, base...), + "--backup-dir="+filepath.ToSlash(fullBackupDir), + "--dry-run", + updateWithUnreproduciblePredicate, + ), env) + require.Equal(t, 0, preview.exitCode, preview.stderr+"\n"+preview.stdout) + assert.Equal(t, connectionsBefore, server.connections.Load(), "dry-run must not connect") + var previewPlan struct { + SafetyCheck struct { + Status string `json:"status"` + ErrorKind string `json:"error_kind"` + Message string `json:"message"` + } `json:"safety_check"` + SQL struct { + BackupKind string `json:"backup_kind"` + BackupReasonCode string `json:"backup_reason_code"` + BackupReason string `json:"backup_reason"` + } `json:"sql"` + WouldConnect bool `json:"would_connect"` + WouldExecute bool `json:"would_execute"` + } + require.NoError(t, json.Unmarshal([]byte(preview.stdout), &previewPlan)) + assert.Equal(t, "blocked", previewPlan.SafetyCheck.Status) + assert.Equal(t, "unreproducible_select", previewPlan.SafetyCheck.ErrorKind) + assert.Contains(t, previewPlan.SafetyCheck.Message, "--allow-full-table-backup") + assert.Equal(t, "table", previewPlan.SQL.BackupKind) + assert.Equal(t, "unreproducible_select", previewPlan.SQL.BackupReasonCode) + assert.Contains(t, previewPlan.SQL.BackupReason, "full-table") + assert.False(t, previewPlan.WouldConnect) + assert.False(t, previewPlan.WouldExecute) + + blocked := runSSHX(t, home, append(append([]string{}, base...), + "--backup-dir="+filepath.ToSlash(fullBackupDir), + updateWithUnreproduciblePredicate, + ), env) + require.Equal(t, 255, blocked.exitCode, blocked.stderr+"\n"+blocked.stdout) + var blockedResult sqlResult + require.NoError(t, json.Unmarshal([]byte(blocked.stdout), &blockedResult)) + assert.Equal(t, "unreproducible_select", blockedResult.ErrorKind) + assert.Contains(t, blockedResult.Error, "--allow-full-table-backup") + require.NotNil(t, blockedResult.Backup, blocked.stdout) + assert.Equal(t, "table", blockedResult.Backup.Kind) + assert.Equal(t, "unreproducible_select", blockedResult.Backup.ReasonCode) + _, err = os.Stat(fullBackupDir) + assert.ErrorIs(t, err, os.ErrNotExist) + got := exec.Command("sqlite3", dbPath, "SELECT name FROM users WHERE id=2;") // #nosec G204 -- isolated fixture query + output, err := got.Output() + require.NoError(t, err) + assert.Equal(t, "second\n", string(output)) + + allowed := runSSHX(t, home, append(append([]string{}, base...), + "--backup-dir="+filepath.ToSlash(fullBackupDir), + "--allow-full-table-backup", + updateWithUnreproduciblePredicate, + ), env) + require.Equal(t, 0, allowed.exitCode, allowed.stderr+"\n"+allowed.stdout) + var allowedResult sqlResult + require.NoError(t, json.Unmarshal([]byte(allowed.stdout), &allowedResult)) + require.NotNil(t, allowedResult.Backup) + assert.Equal(t, "table", allowedResult.Backup.Kind) + fullBackup, err := os.ReadFile(allowedResult.Backup.Path) // #nosec G304 -- explicit opt-in test snapshot under fixture root + require.NoError(t, err) + assert.Contains(t, string(fullBackup), "secret-row-one") + assert.Contains(t, string(fullBackup), "secret-row-two") +} + func TestSQLSQLiteAuditCapturesAuthenticatedPeer(t *testing.T) { if _, err := exec.LookPath("sqlite3"); err != nil { t.Skip("requires the real sqlite3 client")