Conceal stellar env output unless --reveal is used#2597
Conversation
|
Claude finished @fnando's task in 3m 40s —— View job Claude PR Review
SummaryOverall this is a good direction: opt-in disclosure ( Higher-level notes
Nits
Security / correctness check (positive)
Inline comments above for the specific spots. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ea6da22b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adjusts stellar env to avoid leaking secret-bearing environment values by default, adding an explicit --reveal opt-in for printing real values and tightening output escaping to be safer in shell contexts.
Changes:
- Fixes the concealment decision by inverting the allow-list check and removing additional vars from the “visible” list.
- Adds
--revealand updatesstellar envoutput formatting: shell-escapedKEY=valuelisting, and single-key output with control characters neutralized. - Updates CLI help text and the generated
FULL_HELP_DOCS.mdto document the new conceal/reveal behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
FULL_HELP_DOCS.md |
Documents default concealment, single-key behavior, and adds --reveal option to the help output. |
cmd/soroban-cli/src/env_vars.rs |
Inverts concealment logic so only allow-listed vars are shown by default; removes ARCHIVE_URL/RPC_URL from visible list. |
cmd/soroban-cli/src/commands/mod.rs |
Updates top-level command help text for stellar env to describe concealment and --reveal. |
cmd/soroban-cli/src/commands/env/mod.rs |
Implements --reveal, adds shell escaping for listing output, and control-character escaping for single-key output. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @fnando's task in 1m 35s —— View job Claude PR Re-review (post-sync)
Summary of changes since the last reviewCommits
Good iteration on docs and tests. Still open from earlier feedbackA few items from the prior round don't appear to be addressed; flagging in case they were intentionally deferred vs. missed. 1. /// Whether to reveal the value of concealed env vars. By default, concealed env vars are
/// hidden behind a placeholder value.
#[arg(long)]
pub reveal: bool,The actual output is a commented line ( Suggested wording:
2. New
3. Silent miss on concealed single-name lookup — if let Some(name) = &self.name {
if let Some(v) = vars.iter().find(|v| &v.key == name) {
if v.is_revealed() {
println!("{}", escape_control_characters(&v.value));
}
}
return Ok(());
}
if v.is_revealed() {
println!("{}", escape_control_characters(&v.value));
} else {
eprintln!("# {name} is concealed; pass --reveal to print its value");
}Same reasoning applies to the breaking-change angle on 4.
Not a blocker; flagging since 5. Changelog / release-notes entry Removing Codex's dotenv concernCodex flagged that If the output contains Nothing else jumped outThe new logic in · branch: |
|
Claude finished @fnando's task in 58s —— View job Claude PR Re-review (post-merge sync)
SummaryThe synchronize that triggered this run is just a merge of Author responses to prior feedback — acknowledged
Net assessmentNo new issues introduced by the merge. All earlier corrections were applied, and the remaining open items have been thoughtfully triaged by the author with sound reasoning. From my side: LGTM as a focused, well-scoped change. The biggest user-facing footgun remaining is the silent break for --- · branch: |
leighmcculloch
left a comment
leighmcculloch
left a comment
There was a problem hiding this comment.
'reveal' is not overly obvious imo, maybe --reveal-secrets to be more explicit.
What
To reduce churn in env handling,
stellar envnow conceals secret-bearing values by default and adds a--revealflag to print the real values when explicitly requested. Output values are also escaped: the fullKEY=valuelisting is shell-quoted (safe foreval "$(stellar env)"), while a single named lookup prints the raw value with only control characters neutralized (safe for command substitution likeRPC_URL=$(stellar env STELLAR_RPC_URL --reveal)).Why
The allow-list that decides which variables are safe to display had its logic inverted: variables marked "safe to display" were the ones being concealed, while everything else — including values that can embed credentials — was printed in plain text. This meant
stellar envcould leak secrets in logs, screen shares, or shell history. This change fixes the inverted check so credential-bearing values are concealed unless the user opts in with--reveal.Known limitations
N/A