Skip to content

fix: CLI --password-stdin + honor handler exit codes #253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikdarlingdata merged 1 commit into from
Apr 21, 2026
Merged

fix: CLI --password-stdin + honor handler exit codes #253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/PlanViewer.Cli/Commands/AnalyzeCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ public static Command Create(ICredentialService? credentialService = null)

var passwordOption = new Option<string?>(
"--password",
"SQL Server password (bypasses credential store)");
"SQL Server password (bypasses credential store). Visible in process listings — prefer --password-stdin.");

var passwordStdinOption = new Option<bool>(
"--password-stdin",
"Read the SQL Server password from stdin (avoids process-listing exposure). Mutually exclusive with --password.");

var configOption = new Option<string?>(
"--config",
Expand All @@ -118,6 +122,7 @@ public static Command Create(ICredentialService? credentialService = null)
timeoutOption,
loginOption,
passwordOption,
passwordStdinOption,
configOption
};

Expand All @@ -137,7 +142,8 @@ public static Command Create(ICredentialService? credentialService = null)
var trustCert = ctx.ParseResult.GetValueForOption(trustCertOption);
var timeout = ctx.ParseResult.GetValueForOption(timeoutOption);
var login = ctx.ParseResult.GetValueForOption(loginOption);
var password = ctx.ParseResult.GetValueForOption(passwordOption);
var passwordInline = ctx.ParseResult.GetValueForOption(passwordOption);
var passwordStdin = ctx.ParseResult.GetValueForOption(passwordStdinOption);
var configPath = ctx.ParseResult.GetValueForOption(configOption);

// Load analyzer config
Expand All @@ -148,10 +154,20 @@ public static Command Create(ICredentialService? credentialService = null)
server ??= env.GetValueOrDefault("PLANVIEW_SERVER");
database ??= env.GetValueOrDefault("PLANVIEW_DATABASE");
login ??= env.GetValueOrDefault("PLANVIEW_LOGIN");
password ??= env.GetValueOrDefault("PLANVIEW_PASSWORD");
if (!trustCert && env.GetValueOrDefault("PLANVIEW_TRUST_CERT")?.Equals("true", StringComparison.OrdinalIgnoreCase) == true)
trustCert = true;

// Resolve password from --password-stdin, --password, or PLANVIEW_PASSWORD
// (in that order). --stdin for plan XML conflicts with --password-stdin.
if (!PasswordResolver.TryResolve(
passwordInline, passwordStdin, stdin,
env.GetValueOrDefault("PLANVIEW_PASSWORD"),
out var password))
{
Environment.ExitCode = 1;
return;
}

if (server != null)
{
await RunLiveAsync(file, server, database, query, outputDir, estimated,
Expand Down
3 changes: 3 additions & 0 deletions src/PlanViewer.Cli/Commands/CredentialCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ private static Command CreateAddCommand(ICredentialService credentialService)
string password;
if (!string.IsNullOrEmpty(passwordArg))
{
Console.Error.WriteLine(
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential add doesn't expose an explicit --password-stdin flag (it just has the implicit "read from stdin if redirected" path below). For symmetry with analyze / query-store, consider routing this through PasswordResolver or adding the matching flag — otherwise the three commands end up with three slightly different password-input models (explicit flag, implicit redirect, or env). Not a blocker; just inconsistency surface area.

Generated by Claude Code

"Warning: --password is visible in process listings and shell history. " +
"Prefer piping the password into stdin (e.g. `echo hunter2 | planview credential add ...`).");
password = passwordArg;
}
else if (Console.IsInputRedirected)
Expand Down
22 changes: 18 additions & 4 deletions src/PlanViewer.Cli/Commands/QueryStoreCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,11 @@ public static Command Create(ICredentialService? credentialService = null)
"--login", "SQL Server login (bypasses credential store)");

var passwordOption = new Option<string?>(
"--password", "SQL Server password");
"--password", "SQL Server password. Visible in process listings — prefer --password-stdin.");

var passwordStdinOption = new Option<bool>(
"--password-stdin",
"Read the SQL Server password from stdin (avoids process-listing exposure). Mutually exclusive with --password.");

var queryIdOption = new Option<long?>(
"--query-id", "Filter by Query Store query ID");
Expand All @@ -93,7 +97,7 @@ public static Command Create(ICredentialService? credentialService = null)
{
serverOption, databaseOption, topOption, orderByOption, hoursBackOption,
outputDirOption, outputOption, compactOption, warningsOnlyOption, configOption,
authOption, trustCertOption, loginOption, passwordOption,
authOption, trustCertOption, loginOption, passwordOption, passwordStdinOption,
queryIdOption, planIdOption, queryHashOption, planHashOption, moduleOption
};

Expand All @@ -112,7 +116,8 @@ public static Command Create(ICredentialService? credentialService = null)
var auth = ctx.ParseResult.GetValueForOption(authOption);
var trustCert = ctx.ParseResult.GetValueForOption(trustCertOption);
var login = ctx.ParseResult.GetValueForOption(loginOption);
var password = ctx.ParseResult.GetValueForOption(passwordOption);
var passwordInline = ctx.ParseResult.GetValueForOption(passwordOption);
var passwordStdin = ctx.ParseResult.GetValueForOption(passwordStdinOption);
var filterQueryId = ctx.ParseResult.GetValueForOption(queryIdOption);
var filterPlanId = ctx.ParseResult.GetValueForOption(planIdOption);
var filterQueryHash = ctx.ParseResult.GetValueForOption(queryHashOption);
Expand All @@ -122,10 +127,19 @@ public static Command Create(ICredentialService? credentialService = null)
// Load .env file if present (CLI args take precedence)
var env = ConnectionHelper.LoadEnvFile();
login ??= env.GetValueOrDefault("PLANVIEW_LOGIN");
password ??= env.GetValueOrDefault("PLANVIEW_PASSWORD");
if (!trustCert && env.GetValueOrDefault("PLANVIEW_TRUST_CERT")?.Equals("true", StringComparison.OrdinalIgnoreCase) == true)
trustCert = true;

// Resolve password from --password-stdin, --password, or PLANVIEW_PASSWORD
if (!PasswordResolver.TryResolve(
passwordInline, passwordStdin, stdinAlreadyClaimed: false,
env.GetValueOrDefault("PLANVIEW_PASSWORD"),
out var password))
{
Environment.ExitCode = 1;
return;
}

if (top < 1)
{
Console.Error.WriteLine("--top must be >= 1");
Expand Down
63 changes: 63 additions & 0 deletions src/PlanViewer.Cli/PasswordResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
namespace PlanViewer.Cli;

/// <summary>
/// Resolves a SQL Server password from the available CLI inputs:
/// 1. --password-stdin (reads one line from redirected stdin)
/// 2. --password (inline CLI arg; emits a stderr warning because it's
/// visible in process listings, shell history, and audit logs)
/// 3. PLANVIEW_PASSWORD environment variable (from the process environment or
/// a .env file, already looked up by the caller)
/// </summary>
internal static class PasswordResolver
{
/// <summary>
/// Returns true with a resolved password (which may be null if no source provided
/// one). Returns false on user error (mutual-exclusion violation or stdin not
/// redirected when --password-stdin was requested). The caller is responsible
/// for setting Environment.ExitCode on failure.
/// </summary>
public static bool TryResolve(
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New file with five distinct decision branches (mutual-exclusion, stdin-claimed, stdin-not-redirected, stdin-success, inline-with-warning, env-fallback) and no unit tests. The table in the PR description was exercised end-to-end against the built planview.exe, but there's nothing in tests/ pinning this behavior. A small PasswordResolverTests class (injecting a TextReader + bool inputRedirected instead of reading Console.In directly) would catch the kinds of regressions this kind of branchy validation tends to collect.

Generated by Claude Code

string? inlinePassword,
bool passwordFromStdin,
bool stdinAlreadyClaimed,
string? envPassword,
out string? password)
{
password = null;

if (passwordFromStdin && !string.IsNullOrEmpty(inlinePassword))
{
Console.Error.WriteLine("--password and --password-stdin are mutually exclusive.");
return false;
}

if (passwordFromStdin && stdinAlreadyClaimed)
{
Console.Error.WriteLine("--password-stdin can't be combined with --stdin (both read from stdin).");
return false;
}

if (passwordFromStdin)
{
if (!Console.IsInputRedirected)
{
Console.Error.WriteLine("--password-stdin requires stdin to be redirected (pipe the password into the command).");
return false;
}
password = Console.In.ReadLine()?.TrimEnd('\r', '\n') ?? "";
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Console.In.ReadLine() returns null on EOF and "" on an empty line; both collapse to password = "" here and TryResolve returns true. That empty password flows into the connection string and produces a generic SQL auth failure rather than a clear "no password read from stdin" error.

Repro: planview analyze --password-stdin ... < /dev/null (or piping an empty file) silently attempts to connect with an empty password instead of failing at the CLI boundary.

Consider failing explicitly when the read yields null-or-empty, since --password-stdin means "I'm providing a password via stdin" — a zero-byte read is almost certainly a scripting mistake.

Generated by Claude Code

return true;
}

if (!string.IsNullOrEmpty(inlinePassword))
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subtle behavior change from string.IsNullOrEmpty here: the old code was password ??= env.GetValueOrDefault("PLANVIEW_PASSWORD"), which only fell back to the env var when --password was unset (null). Now --password "" (empty string) falls through to envPassword as well.

Pre-change: --password "" with PLANVIEW_PASSWORD=foo → connects with "".
Post-change: same invocation → connects with foo.

Almost certainly nobody depends on passing an empty literal, but worth being aware it's a behavior delta, not just a refactor.

Generated by Claude Code

{
Console.Error.WriteLine(
"Warning: --password is visible in process listings and shell history. " +
"Prefer --password-stdin, the PLANVIEW_PASSWORD env var, or the credential store.");
password = inlinePassword;
return true;
}

password = envPassword;
return true;
}
}
6 changes: 5 additions & 1 deletion src/PlanViewer.Cli/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,8 @@
if (credentialService != null)
root.Add(CredentialCommand.Create(credentialService));

return await root.InvokeAsync(args);
// System.CommandLine's InvokeAsync returns 0 for successful dispatch even when a
// handler set Environment.ExitCode = 1 to signal a validation error. Honor either
// signal so scripts can tell success from failure.
var code = await root.InvokeAsync(args);
return code != 0 ? code : Environment.ExitCode;
Copy link
Copy Markdown
Owner Author

@erikdarlingdata erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression code != 0 ? code : Environment.ExitCode is asymmetric: if a handler sets Environment.ExitCode = 2 and InvokeAsync returns non-zero for an unrelated reason (e.g. parser error), the handler's signal is dropped. Today no handler uses a code other than 1 so it's fine, but the comment says "honor either signal" — it actually prefers the framework's signal. Minor; leaving a note in case this grows teeth later.

Generated by Claude Code

Loading