Fix CLI exit paths to preserve Zeroize semantics#29
Conversation
Change read_password() to return Result<Zeroizing<String>, (ExitCode, String)> instead of calling process::exit() internally. This allows the Zeroize Drop implementation to run on password buffers when errors occur, preventing sensitive data from remaining in memory on early exit paths.
Reviewer's GuideRefactors CLI password reading to return structured error results instead of exiting the process directly, so Zeroizing drop semantics are preserved on all error paths and errors are surfaced consistently to run(). Sequence diagram for CLI password reading and error handling refactorsequenceDiagram
participant Run as run
participant RPC as read_password_confirmed
participant RP as read_password
participant FS as fs_read
participant ENV as env_var
participant RPP as rpassword_prompt_password
participant JSON as emit_json
Run->>RPC: read_password_confirmed(password_file)
RPC->>RP: read_password(password_file)
alt password_file provided
RP->>FS: fs::read(path)
alt fs::read error
FS-->>RP: Err
RP-->>RPC: Err(ExitCode::BadInput, msg)
RPC-->>Run: Err(ExitCode::BadInput, msg)
else fs::read ok
FS-->>RP: Ok(bytes)
RP-->>RPC: Ok(Zeroizing<String>)
RPC-->>Run: Ok(Zeroizing<String>)
end
else NEURON_PASSWORD set
RP->>ENV: std::env::var(NEURON_PASSWORD)
ENV-->>RP: Ok(pw)
RP-->>RPC: Ok(Zeroizing<String>)
RPC-->>Run: Ok(Zeroizing<String>)
else interactive
RP->>RPP: rpassword::prompt_password
alt prompt_password error
RPP-->>RP: Err(e)
RP-->>Run: Err(ExitCode::BadInput, msg)
else prompt_password ok
RPP-->>RP: Ok(pw)
RP-->>Run: Ok(Zeroizing<String>)
end
end
%% Error handling path in run for direct read_password use
Run->>RP: read_password(password_file)
alt read_password returns Err
RP-->>Run: Err(code, msg)
alt [cli.json]
Run->>JSON: emit_json(JsonResult{status:"error", error:Some(msg)})
end
Run->>Run: return Err(code)
else read_password returns Ok
RP-->>Run: Ok(Zeroizing<String>)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 59 minutes and 34 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using
(ExitCode, String)as the error type makes the interfaces harder to read and evolve; consider introducing a small error enum or type alias (e.g.,type CliError = (ExitCode, String)orenum CliError { ... }) to clarify intent and avoid tuple unpacking everywhere. - The JSON vs non-JSON error reporting logic added in the
read_passworderror path duplicates patterns used elsewhere; consider factoring this into a small helper (e.g.,fn report_error(code, msg, json, start)), so all exit paths handle formatting and emission consistently. - Now that
read_passwordandread_password_confirmedshare the same error shape, it might be worth extracting shared error messages/constants (e.g., forError reading passphrase) to reduce string duplication and keep future changes to those messages in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `(ExitCode, String)` as the error type makes the interfaces harder to read and evolve; consider introducing a small error enum or type alias (e.g., `type CliError = (ExitCode, String)` or `enum CliError { ... }`) to clarify intent and avoid tuple unpacking everywhere.
- The JSON vs non-JSON error reporting logic added in the `read_password` error path duplicates patterns used elsewhere; consider factoring this into a small helper (e.g., `fn report_error(code, msg, json, start)`), so all exit paths handle formatting and emission consistently.
- Now that `read_password` and `read_password_confirmed` share the same error shape, it might be worth extracting shared error messages/constants (e.g., for `Error reading passphrase`) to reduce string duplication and keep future changes to those messages in one place.
## Individual Comments
### Comment 1
<location path="neuron-encrypt/src/bin/cli.rs" line_range="432-435" />
<code_context>
{
match read_password_confirmed(&cli.password_file) {
Ok(password) => password,
- Err(msg) => {
+ Err((code, msg)) => {
eprintln!("Error: {msg}");
- return Err(ExitCode::BadInput);
</code_context>
<issue_to_address>
**issue (bug_risk):** JSON mode does not emit structured error output for `read_password_confirmed`, unlike the non-confirm path.
In this branch, `read_password_confirmed` errors only print to stderr and return an exit code, even when `cli.json` is set, whereas the non-confirm branch now emits a JSON error before returning. This inconsistency means JSON-mode callers get structured errors only for some password failures (e.g., mismatched passphrases). Please mirror the JSON emission here so all password-related errors produce a JSON payload when `--json` is enabled.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| { | ||
| match read_password_confirmed(&cli.password_file) { | ||
| Ok(password) => password, | ||
| Err(msg) => { | ||
| Err((code, msg)) => { |
There was a problem hiding this comment.
issue (bug_risk): JSON mode does not emit structured error output for read_password_confirmed, unlike the non-confirm path.
In this branch, read_password_confirmed errors only print to stderr and return an exit code, even when cli.json is set, whereas the non-confirm branch now emits a JSON error before returning. This inconsistency means JSON-mode callers get structured errors only for some password failures (e.g., mismatched passphrases). Please mirror the JSON emission here so all password-related errors produce a JSON payload when --json is enabled.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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: 1b8ed6053a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut pw = Zeroizing::new(String::from_utf8(bytes).map_err(|e| { | ||
| (ExitCode::BadInput, format!("Password file contains invalid UTF-8: {e}")) | ||
| })?); |
There was a problem hiding this comment.
When --password-file points at a file whose secret contains any invalid UTF-8 byte, String::from_utf8 returns the original file contents inside FromUtf8Error; this handler formats the error and drops that buffer before it is ever wrapped in Zeroizing. That leaves the password file bytes in the allocator on this new non-exiting error path, so the hardening does not actually wipe the sensitive buffer for invalid password files.
Useful? React with 👍 / 👎.
Code Review Roast 🔥Verdict: 1 Issue Found | Recommendation: Address before merge Overview
details:
🏆 Best part: The core security fix is solid — removing those 💀 Worst part: Line 435 — the 📊 Overall: Like teaching a kid to tie their shoes but forgetting to show them the left foot — the intent is good, but half the implementation missed the memo. Files Reviewed (1 file)
|
Code Review Roast 🔥Verdict: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The core security fix is solid — removing those 💀 Worst part: Line 435 — the 📊 Overall: Like teaching a kid to tie their shoes but forgetting to show them the left foot — the intent is good, but half the implementation missed the memo. Files Reviewed (1 file)
Reviewed by laguna-m.1-20260312:free · Input: 284.6K · Output: 10.3K · Cached: 66K |
1 participant
Change read_password() to return Result<Zeroizing, (ExitCode, String)> instead of calling process::exit() internally. This allows the Zeroize Drop implementation to run on password buffers when errors occur, preventing sensitive data from remaining in memory on early exit paths.
Summary by Sourcery
Adjust CLI password-reading flow to propagate errors instead of exiting directly so that Zeroize cleanup can occur on all exit paths.
Bug Fixes:
Enhancements: