Skip to content

Say on stderr which settings came from a .env file - #588

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/cli-env-notice
Sep 28, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/cli-env-notice

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

The CLI reads a .env file from the working directory for analyze and query-store. That file can pick the server and turn off certificate validation. The CLI applied those settings without a word. So if a cloned repo held a .env file, planview analyze connected to the server that the file named, and nothing told the user.

Now the CLI prints one line on stderr when the .env file supplies a setting. The line names the file and the settings, and it never shows their values:

Using settings from C:\work\.env: PLANVIEW_SERVER, PLANVIEW_DATABASE, PLANVIEW_LOGIN, PLANVIEW_TRUST_CERT, PLANVIEW_PASSWORD

The line lists only the settings that had an effect:

  • A setting that a command-line option overrode is not listed.
  • With no server from either place, analyze reads the plan file offline and takes nothing from the file.
  • PLANVIEW_PASSWORD is used only with a login. Without a login, the command uses the credential store or Windows authentication, as before.

If a PLANVIEW_ value holds a control character, the command stops. The error names the setting and the file, but not the value. The CLI prints the server and database names later. An escape sequence in them moves the cursor and erases text, such as the new line.

No real setting needs a control character, but tab is allowed. The file path in the line and in the error shows any control character as ?.

The automatic load does not change, so existing .env files keep working. Apart from the new line and the control-character check, the commands behave as before.

How it works:

  • ConnectionHelper.LoadEnvFile now returns an EnvFile object. Both commands call one method, EnvFile.Fill. It fills only the settings that the command line left out, and it records each key that it supplied.
  • EnvFile.PasswordFor gives the file's password only when there is a server and a login.
  • PasswordResolver.TryResolve takes the .env password as a function. It calls the function only when neither --password-stdin nor --password gave a password, so the list is exact. It also takes an optional writer for its messages, so the tests do not replace Console.Error.
  • The resolver's doc comment and its --password warning said "PLANVIEW_PASSWORD env var". The CLI never read a process environment variable, only the .env file. The text now says "PLANVIEW_PASSWORD in a .env file".
  • The README section on .env files describes the new line, when the file's settings apply, and the control-character check.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

  • New EnvFileTests, 20 cases:
    • With every setting from the file, the line lists all five keys in order, and the file path is a full path.
    • For each of the server, database, login and trust-cert settings, the command-line option wins and its key is not listed.
    • With no server, nothing is taken from the file and there is no line.
    • The file's password is used only with a login, and only when no option gave a password. --password still writes its warning.
    • The line never contains a value. PLANVIEW_TRUST_CERT=false changes nothing and is not listed. A folder with no .env file supplies nothing.
    • ESC, CSI (U+009B), BEL, NUL and DEL in a value each stop the command. The error names the key and the file, and it holds no value and no control character.
    • Tab is allowed. Keys that do not start with PLANVIEW_ are not checked. A control character in the path shows as ?.
  • The built planview.exe ran against SQL Server 2025 from folders with a .env file:
    • analyze with every setting from the file listed all five keys, and the capture succeeded.
    • analyze with --server and --trust-cert listed only the database, login and password.
    • query-store listed the login, trust-cert and password, and it analyzed a plan from Query Store.
    • A database name that held two escape sequences stopped analyze with exit code 1. Stderr held no escape character.
    • analyze of a .sqlplan file, with no server in the .env file, printed no line.
    • With no login in the file, the password was not listed.
  • Full suite on Windows: 944 total, 943 passed, 0 failed, 1 skipped. The Release build has 0 warnings.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build, --no-incremental)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR

erikdarlingdata and others added 2 commits September 28, 2026 08:46
A .env file in the working directory can pick the server and turn off
certificate validation for analyze and query-store. The CLI applied those
settings without a word. Now it prints one line on stderr that names the
file and the settings it supplied, never their values. A setting that a
command-line option overrode is not listed.

PasswordResolver asks for the .env password only when neither
--password-stdin nor --password gave one, so the list is exact. Its doc
comment and --password warning no longer mention a PLANVIEW_PASSWORD
environment variable, which the CLI never read.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
…characters

Review round 1 on #588:

- A PLANVIEW_ value with a control character now stops the command with an
  error that names the key and the file, never the value. The CLI prints the
  server and database later, and an escape sequence there could erase the
  notice. The file path and keys in the notice and the error show control
  characters as '?'.
- Both commands merge the file through one EnvFile.Fill method. With no
  server, analyze runs offline and takes nothing from the file. The file's
  password is used only with a login, so the notice lists only settings that
  had an effect.
- PasswordResolver.TryResolve takes an optional writer for its messages, so
  the tests no longer swap Console.Error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 13:18
@erikdarlingdata
erikdarlingdata merged commit 5b7a6c7 into dev Sep 28, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/cli-env-notice branch September 28, 2026 13:18
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed. This is a clean, well-scoped fix for a real issue (a repo-supplied .env silently picking the server / disabling cert validation).

A few things I checked closely and found correct:

  • Ordering/precedence: Fill() only calls Use() when the command-line value is null (or, for TrustCert, only when it's still false), so CLI args always win and the notice only ever lists keys that actually changed behavior. Verified this holds for both AnalyzeCommand (server optional → offline mode) and QueryStoreCommand (server/database required, so only login/trust-cert/password can come from the file).
  • Password sequencing: PasswordFor is invoked lazily via the Func<string?> passed into PasswordResolver.TryResolve, so it's only called (and only recorded into the notice) when neither --password-stdin nor --password supplied one — matches the "list is exact" claim in the PR description.
  • No value leakage: the control-character check in EnvFile's constructor scans all PLANVIEW_* values (including the password), and both Error and Notice only ever interpolate key names and the sanitized path, never a value.
  • Terminal-injection defense: rejecting any control character other than \t (including CSI 0x9B, not just ESC) is appropriately conservative for stderr text that could otherwise reposition the cursor/erase the notice line itself.
  • API consumers: PasswordResolver.TryResolve and ConnectionHelper.LoadEnvFile are both fully migrated at their only call sites — no leftover callers on the old Dictionary<string,string> / string? envPassword signatures.

No correctness, security, or convention issues found. Not applicable here: no T-SQL, no PlanViewer.Ssms/PlanViewer.Web files touched, so the version-bump and linked-Compile-Include conventions don't come into play.

@erikdarlingdata erikdarlingdata mentioned this pull request Sep 29, 2026
2 of 8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant