From 78f58639475dc6f14ccd214eaa6f1b7d6847b742 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Thu, 22 Oct 2026 20:00:52 -0700 Subject: [PATCH] case 4690 - backup: run pg_dump without a shell, password in PGPASSWORD, failures detected by exit code Co-Authored-By: Claude Opus 5.5 --- devdocs/modernization/NET10-PORT.md | 4 +- server/AyaNova/generator/CoreJobBackup.cs | 41 ++++++++--------- server/AyaNova/util/RunProgram.cs | 54 +++++++++++++++++++++++ 3 files changed, 78 insertions(+), 21 deletions(-) diff --git a/devdocs/modernization/NET10-PORT.md b/devdocs/modernization/NET10-PORT.md index 21a873aa..acc5e1a4 100644 --- a/devdocs/modernization/NET10-PORT.md +++ b/devdocs/modernization/NET10-PORT.md @@ -115,7 +115,7 @@ in this branch or a follow-up release. UPDATE: John has decided these all go in - [x] **Case 4676: data-list SQL injection — priority.** Confirmed: `DataListSqlFilterCriteriaBuilder` appended the raw filter value for Roles and for every numeric type (Enum, MemorySize, Decimal, Currency, InternalId, Integer). Text and tag values escape `'`; TimeSpan goes through `int.Parse`. Any authenticated user who can open a data list could reach this. **Sockeye's fix (`3427cd8`) is incomplete:** it checks only Decimal/Currency and Integer, and leaves InternalId, Enum, MemorySize and Roles open (still open in Sockeye 8.1.0). AyaNova's fix parses every numeric type as an invariant `decimal` (Decimal, Currency) or `long` (the rest), and Roles as a `long`, then appends the re-formatted number. An invalid value throws `ArgumentException`, which the API returns as an error before any SQL runs - [x] **`DbUtil.AdminConnectionString`.** Fixed with the builder below (Sockeye 8.1.0 still has the `Replace`). Confirmed: `DbUtil.cs:99` is `_dbConnectionString.Replace(_dbName, "postgres")`, used by the boot check (`:192`), create (`:286`) and drop/recreate (`:334`). Any customer whose role name, password or host contains the database name as text gets a mangled admin connection, and the boot check authenticates as the wrong user. Fix: `new NpgsqlConnectionStringBuilder(cs) { Database = "postgres" }.ConnectionString` -- [ ] **Backup `pg_dump` call.** Confirmed: `CoreJobBackup.cs:68` builds `postgresql://user:password@host:port/db` with the raw password, and `:94` adds `-Fc > file`; `RunProgram` runs it through `cmd.exe /C` (`RunProgram.cs:55`) or `/bin/sh -c` (`:82-89`). A password with spaces or `@ : / ? # %` breaks backups. Also, on Linux stderr is not redirected (`RunProgram.cs:86-92`), so a failed pg_dump returns empty output and is **reported as a successful backup** (`CoreJobBackup.cs:97`). Fix: password in `PGPASSWORD` in the process environment, connection parameters as separate arguments through `ProcessStartInfo.ArgumentList` (no shell), output with `-f`, stderr and exit code checked +- [x] **Backup `pg_dump` call.** Fixed (Sockeye 8.1.0 still has the old call): new `RunProgram.RunNoShell` (no shell, `ArgumentList`, per-process environment, both streams read together, timeout kills the process tree, non-zero exit code returned as an error); backup passes `--host/--port/--username/--dbname/--format=custom/--file/--no-password` with the password in `PGPASSWORD`. The old `RunProgram.Run` now has no callers (see Deferred). Confirmed: `CoreJobBackup.cs:68` builds `postgresql://user:password@host:port/db` with the raw password, and `:94` adds `-Fc > file`; `RunProgram` runs it through `cmd.exe /C` (`RunProgram.cs:55`) or `/bin/sh -c` (`:82-89`). A password with spaces or `@ : / ? # %` breaks backups. Also, on Linux stderr is not redirected (`RunProgram.cs:86-92`), so a failed pg_dump returns empty output and is **reported as a successful backup** (`CoreJobBackup.cs:97`). Fix: password in `PGPASSWORD` in the process environment, connection parameters as separate arguments through `ProcessStartInfo.ArgumentList` (no shell), output with `-f`, stderr and exit code checked - [ ] **Drop-and-recreate database.** Confirmed: `DbUtil.cs:342-345` drops, then creates; triggered by `AYANOVA_PERMANENTLY_ERASE_DATABASE` (`Startup.cs:567-570`). Without `CREATEDB` it drops the database and then fails to create it. Test the permission before dropping, or change the order - [ ] **Report browser download location.** Confirmed: `new BrowserFetcher()` with the default path beside the app (`ReportBiz.cs:800`). Point it under the data directory in code. Beside the app it is read-only under a hardened systemd unit (`ProtectSystem=strict`), and a redeploy that replaces the app folder deletes it - [ ] **Report failures: return a specific error.** The Sockeye crash is n/a: rendering runs as a job, and `JobsBiz.ProcessJobAsync` catches every exception, logs it through NLog and marks the job Failed (`JobsBiz.cs:390-439`), so the process survives. What remains: a browser download or launch failure (`ReportBiz.cs:800`, `:838`, outside the render `try`) reaches the client only as a generic job failure. Catch it in `DoRenderJob` and return a render error that says what failed @@ -183,6 +183,7 @@ Found during the port, out of scope. One line each. - **Race, affects production after every restart:** `FormFieldReference.FormFieldReferenceList` (`biz/FormFieldReference.cs:57-59`) builds a static dictionary lazily with no lock, assigning it before filling it. A concurrent first request sees it half-built and throws `FormFieldReferenceList: is not valid`. Seen twice on .NET 10, each time on the first form-field use after boot: `VendorCrud.CRUD`, then `FormCustom.FormCustomUpdate`. Fix: build the dictionary fully before publishing it (`Lazy` or a static initialiser). Pre-existing, not a port change. John's decision: its own case, fixed with the other cases going into this release, not under the port - `raven-launcher` `Program.cs:98` calls `PGProcess.StandardInput.Close()` but standard input was never redirected, so it throws `InvalidOperationException` at shutdown: the "Postgres shut down" step never runs and the launcher exits with an error when the server stops. Postgres was started detached by `pg_ctl start`, so it probably stays running. Pre-existing; candidate for its own case - Sockeye 8.1.0 has the NLog 6 archive naming problem fixed here for AyaNova: archives are `log-sockeye_NN.txt`, old `-0` to `-8` archives are never deleted, and its log viewer still sorts by name (it also filters on `ayanova.txt`, not `sockeye.txt`, so the current file is listed twice). Same fix applies: `ArchiveSuffixFormat = "-{0}"` and the date sort +- `RunProgram.Run` (and its `RunWindows`/`RunLinuxShell`/`RunOSX` helpers) has no callers since the backup moved to `RunNoShell`. It runs through `cmd.exe /C` or `/bin/sh -c` and logs its full command line on error: delete it rather than reuse it - **Improvement, own case after the port (John to create):** bring back logging of JavaScript error details from report page console errors, as Sockeye's port did. AyaNova logs only `ERROR: {message text}`; the per-argument detail (`arg instanceof Error ? arg.message : arg`) is commented out at `biz/ReportBiz.cs:873-876` since case 4586, because `IJSHandle.ExecutionContext` was removed. Sockeye 8.1.0 (`server/biz/ReportBiz.cs:575`) evaluates on the handle instead: `args.Message.Args.Select(arg => arg.EvaluateFunctionAsync("(arg) => arg instanceof Error ? arg.message : arg"))`, which also needs the console handler to be `async` again ## Log @@ -206,3 +207,4 @@ Found during the port, out of scope. One line each. | 2026-10-22 | MiniProfiler removed | Builds (only AV0015/AV0016/CA2022). No `StackExchange.Profiling`/`MiniProfiler`/`AY_PROFILER` left in the server. `/profiler/results-index` now gets the client app's `index.html` fallback like any unknown path; unauthenticated API call still 401. Integration suite = baseline | | 2026-10-22 | CA2022: `FileUtil.GetLinkerTimestampUtc` reads with `ReadAtLeast` | CA2022 gone (only AV0015/AV0016 left). Licence gate 730/730 incl. build-timestamp and `MExBB` checks; per-row outcomes identical to the first .NET 10 run (389 ok / 337 rejected for this build). Server boots and accepts the (trial) licence | | 2026-10-22 | Bogus 35.6.5, jose-jwt 5.3.0, QRCoder 1.8.1, Newtonsoft.Json 13.0.4 direct; `appsettings.json` levels; ASP0019 n/a | Builds (only AV0015/AV0016). All packages current, none vulnerable. Login JWT still `HS256` and accepted on every authenticated call; `GET auth/totp` returns a valid 183×183 QR PNG and leaves 2FA disabled. Integration suite = baseline. Bogus (evaluation seeder) compiled only: running the seeder replaces the test data | +| 2026-10-22 | Backup `pg_dump` without a shell; password in `PGPASSWORD` | On-demand backups through the API into a scratch folder with a space in its name, checked with `pg_restore --list`. Old code as `postgres`: 803 entries. New code as `postgres`: 803 entries. Failing stand-in `pg_dump` (exit code 2): reported as `BACKUP ERROR`, no file, no password in the log. Temporary superuser role `"AyaNova"` (named like the database) with password `p @:/?#% x`: old code failed (`too many command-line arguments (first is "@:/?#%")`), left a 0-byte backup file and echoed part of the password into the log; new code 803 entries, no errors. Role dropped afterwards. Integration suite = baseline. Linux not tested here: the stderr gap there is closed by the same code (no shell, exit code checked) | diff --git a/server/AyaNova/generator/CoreJobBackup.cs b/server/AyaNova/generator/CoreJobBackup.cs index a99745f2..7de4df0f 100644 --- a/server/AyaNova/generator/CoreJobBackup.cs +++ b/server/AyaNova/generator/CoreJobBackup.cs @@ -59,13 +59,13 @@ namespace AyaNova.Biz // var DemandFileNamePrepend = OnDemand ? "manual-" : string.Empty; //************* //DO DATA BACKUP - //build command - //this is valid on windows - //C:\data\code\PostgreSQLPortable_12.0\App\PgSQL\bin\pg_dump --dbname=postgresql://postgres:raven@127.0.0.1:5432/AyaNova -Fc > huge_new.backup + //run pg_dump directly with no shell: each connection detail is its own argument and the password goes in PGPASSWORD, + //so a user name or password containing spaces or @ : / ? # % can't break the command (it used to be a postgresql:// URI + //with the raw password, run through cmd.exe or /bin/sh for a > file redirect), and the password is never on a command line + //equivalent to: pg_dump --host=localhost --port=5432 --username=postgres --dbname=AyaNova --format=custom --file=db-xxx.backup // await JobsBiz.LogJobAsync(Guid.Empty, $"Data backup starting"); Npgsql.NpgsqlConnectionStringBuilder PostgresConnectionString = new Npgsql.NpgsqlConnectionStringBuilder(ServerBootConfig.AYANOVA_DB_CONNECTION); - var DBNameParameter = $"--dbname=postgresql://{PostgresConnectionString.Username}:{PostgresConnectionString.Password}@{PostgresConnectionString.Host}:{PostgresConnectionString.Port}/{PostgresConnectionString.Database}"; //var DataBackupFile = $"{DemandFileNamePrepend}db-{FileUtil.GetSafeDateFileName()}.backup";//presentation issue so don't use UTC for this one var DataBackupFile = $"db-{FileUtil.GetSafeDateFileName()}.backup";//presentation issue so don't use UTC for this one @@ -77,23 +77,24 @@ namespace AyaNova.Biz if (!string.IsNullOrWhiteSpace(ServerBootConfig.AYANOVA_BACKUP_PG_DUMP_PATH)) BackupUtilityCommand = Path.Combine(ServerBootConfig.AYANOVA_BACKUP_PG_DUMP_PATH, BackupUtilityCommand); - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - //put quotes around paths if spaces in them - if (BackupUtilityCommand.Contains(' ')) - { - BackupUtilityCommand = $"\"{BackupUtilityCommand}\""; - } + var Arguments = new List(); + //host and user only when set, otherwise pg_dump's own defaults apply as they would for any omitted option + if (!string.IsNullOrWhiteSpace(PostgresConnectionString.Host)) + Arguments.AddRange(new[] { "--host", PostgresConnectionString.Host }); + Arguments.AddRange(new[] { "--port", PostgresConnectionString.Port.ToString() }); + if (!string.IsNullOrWhiteSpace(PostgresConnectionString.Username)) + Arguments.AddRange(new[] { "--username", PostgresConnectionString.Username }); + Arguments.AddRange(new[] { + "--dbname", PostgresConnectionString.Database, + "--format=custom", + "--file", DataBackupFile, + "--no-password"//never prompt, a missing or wrong password must fail rather than hang + }); + var PgEnvironment = new Dictionary(); + if (!string.IsNullOrEmpty(PostgresConnectionString.Password)) + PgEnvironment["PGPASSWORD"] = PostgresConnectionString.Password; - if (DataBackupFile.Contains(' ')) - { - DataBackupFile = $"\"{DataBackupFile}\""; - } - } - - var Arguments = $"{DBNameParameter} -Fc > {DataBackupFile}"; - - var Result = RunProgram.Run(BackupUtilityCommand, Arguments, log, MAXIMUM_MS_ALLOWED_FOR_PROCESSING); + var Result = RunProgram.RunNoShell(BackupUtilityCommand, Arguments, PgEnvironment, log, MAXIMUM_MS_ALLOWED_FOR_PROCESSING); if (!string.IsNullOrWhiteSpace(Result)) { var msg = $"Error during data backup \"{Result}\""; diff --git a/server/AyaNova/util/RunProgram.cs b/server/AyaNova/util/RunProgram.cs index a5b63367..63057d4e 100644 --- a/server/AyaNova/util/RunProgram.cs +++ b/server/AyaNova/util/RunProgram.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.IO; using System.Runtime.InteropServices; using Microsoft.Extensions.Logging; @@ -46,6 +47,59 @@ namespace AyaNova.Util + /// + /// Run a program directly, no shell: each argument is passed as-is (no quoting or escaping needed, nothing is interpreted by a shell) + /// and extra environment variables are set for this process only (e.g. PGPASSWORD so a password never appears on a command line) + /// Returns empty string on success (exit code 0) or the error text otherwise, same convention as the backup job expects + /// + public static string RunNoShell(string fileName, IEnumerable arguments, IDictionary environment = null, ILogger log = null, int waitForExitTimeOut = int.MaxValue) + { + try + { + using (var process = new Process()) + { + process.StartInfo = new ProcessStartInfo + { + FileName = fileName, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + }; + foreach (var a in arguments) + process.StartInfo.ArgumentList.Add(a); + if (environment != null) + foreach (var e in environment) + process.StartInfo.Environment[e.Key] = e.Value; + + process.Start(); + //read both streams at once, reading one to the end first can deadlock if the other fills its buffer + var stdOut = process.StandardOutput.ReadToEndAsync(); + var stdErr = process.StandardError.ReadToEndAsync(); + if (!process.WaitForExit(waitForExitTimeOut)) + { + process.Kill(true); + return $"ERROR: TIMED OUT {waitForExitTimeOut}ms BEFORE COMPLETION running {fileName}"; + } + process.WaitForExit();//ensures output streams are fully read after the timed wait above + if (process.ExitCode != 0) + return $"ERROR: {Path.GetFileName(fileName)} exit code {process.ExitCode}: {stdErr.Result}{stdOut.Result}".Trim(); + return string.Empty; + } + } + catch (Exception ex) + { + //arguments deliberately not logged, they can contain connection details + if (log != null) + { + log.LogError(ex, $"RunProgram error running command:{fileName}"); + } + throw; + } + } + + + private static string RunWindows(string cmd, string arguments, int waitForExitTimeOut = int.MaxValue) { //RunProgram.Run("cmd.exe",FullRunCommand, log);