feat: support MOSTRO_NSEC_PRIVKEY env var for Nostr private key#713
Conversation
Add MOSTRO_NSEC_PRIVKEY environment variable to allow storing the Nostr private key separately from settings.toml. The variable can be set via real environment, ~/.mostro/.env (auto-loaded at startup), or systemd/Docker configuration. Changes: - Add dotenvy dependency for .env file loading - Implement load_env_file() to auto-load <settings_dir>/.env at startup - Implement apply_nsec_env_override() to read MOSTRO_NSEC_PRIVKEY and override
…ent variable - Mark NostrSettings.nsec_privkey as optional with #[serde(default)] - Remove "I'll configure MOSTRO_NSEC_PRIVKEY myself" option from setup wizard - Export MOSTRO_NSEC_PRIVKEY to current process when saving to .env so daemon can use it immediately without restart - Add test verifying settings.toml parses without nsec_privkey field when relying on environment variable
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds optional ChangesNostr nsec env/.env support
sequenceDiagram
participant Wizard as Wizard (setup)
participant Process as Process Env
participant FS as Filesystem (<settings_dir>/.env)
participant Config as Config Loader (init_configuration_file)
participant Settings as settings.toml
Wizard->>FS: optionally write MOSTRO_NSEC_PRIVKEY=<nsec> to .env (write_env_file)
Wizard->>Process: set MOSTRO_NSEC_PRIVKEY in-process
Config->>FS: load optional .env (dotenvy::from_path)
Config->>Process: read MOSTRO_NSEC_PRIVKEY (trim, ignore empty)
alt env present
Config->>Settings: override parsed/created nsec_privkey with env value
else env absent
Config->>Settings: use settings.toml nsec_privkey (may be empty)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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: dd9e3e33be
ℹ️ 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".
| std::fs::OpenOptions::new() | ||
| .write(true) | ||
| .create(true) | ||
| .truncate(true) | ||
| .mode(0o600) |
There was a problem hiding this comment.
On Unix, OpenOptionsExt::mode(0o600) only applies when the file is newly created; it does not tighten permissions on an already-existing file. If .env already exists with broader bits (for example 0644), this path truncates and rewrites it but leaves those broad permissions in place, exposing MOSTRO_NSEC_PRIVKEY to other local users. After opening/writing, explicitly set permissions to 0o600 (or recreate the file) so the secret is always protected.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/config/wizard.rs (2)
208-208: Reuse the env-var name and.envfilename from shared constants.Both
"MOSTRO_NSEC_PRIVKEY"(line 225) and".env"(line 208) are duplicated here after being introduced asNSEC_ENV_VAR/ENV_FILENAMEinsrc/config/util.rs. Centralising them insrc/config/constants.rs(and re-using from both modules) prevents drift if the name ever changes.♻️ Suggested approach
Promote them in
src/config/constants.rs:pub const ENV_FILENAME: &str = ".env"; pub const NSEC_ENV_VAR: &str = "MOSTRO_NSEC_PRIVKEY";Then update
util.rsandwizard.rsto import fromcrate::config::constants::{ENV_FILENAME, NSEC_ENV_VAR}instead of defining/hardcoding them locally.- let env_file_path = settings_dir.join(".env"); + let env_file_path = settings_dir.join(ENV_FILENAME); @@ - std::env::set_var("MOSTRO_NSEC_PRIVKEY", nsec); + std::env::set_var(NSEC_ENV_VAR, nsec); @@ - writeln!(file, "MOSTRO_NSEC_PRIVKEY={}", nsec) + writeln!(file, "{}={}", NSEC_ENV_VAR, nsec)Also applies to: 225-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/wizard.rs` at line 208, Replace the hardcoded ".env" and "MOSTRO_NSEC_PRIVKEY" occurrences with the shared constants ENV_FILENAME and NSEC_ENV_VAR from crate::config::constants: update wizard.rs (where env_file_path is built and where the NSEC env var is referenced) and util.rs to import crate::config::constants::{ENV_FILENAME, NSEC_ENV_VAR} instead of defining or hardcoding these values locally, and remove the duplicate local definitions so both modules reuse the single source of truth.
247-270:write_env_file: permission + truncation edge cases.Two small robustness gaps in the env-file writer:
.mode(0o600)only applies when the file is created. If~/.mostro/.envalready exists with looser permissions,OpenOptions::openpreserves them — nochmodtakes effect. Callingstd::fs::set_permissionsafter opening guarantees 0o600 regardless..truncate(true)discards any unrelated entries the operator might have added to.env. In the current flow this is called only during initial wizard setup (whensettings.tomlis being created), so collisions are unlikely, but an append-or-replace strategy would be safer long-term.🛡️ Proposed hardening for (1)
let mut file = file.map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + } writeln!(file, "MOSTRO_NSEC_PRIVKEY={}", nsec) .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/wizard.rs` around lines 247 - 270, In write_env_file, ensure file permissions are enforced after opening by calling std::fs::set_permissions(path, Permissions::from_mode(0o600)) (or the cross-platform equivalent) so existing files get 0o600 even when not created; and avoid blind truncate by implementing an "append-or-replace" flow: if the file exists, read its contents, replace any line starting with "MOSTRO_NSEC_PRIVKEY=" with the new value (or append the line if missing), then write the updated content back (you may truncate when writing the final assembled content). Keep these changes localized to the write_env_file function so it still uses OpenOptions for creation but then enforces permissions and safely updates the MOSTRO_NSEC_PRIVKEY entry.src/config/util.rs (3)
21-26: Consider logging when.envload fails.Silently discarding the
dotenvy::from_pathresult will hide misconfigurations (e.g., malformed.envor permission errors on the file) from operators. A singletracing::warn!keeps behavior backward-compatible but makes troubleshooting easier.🔎 Proposed fix
fn load_env_file(settings_dir: &std::path::Path) { let env_file = settings_dir.join(ENV_FILENAME); if env_file.exists() { - let _ = dotenvy::from_path(&env_file); + if let Err(err) = dotenvy::from_path(&env_file) { + tracing::warn!( + path = %env_file.display(), + error = %err, + "Failed to load .env file" + ); + } } }As per coding guidelines: "Prefer
tracingspans over ad-hoc logging in Rust code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/util.rs` around lines 21 - 26, The function load_env_file currently discards the result of dotenvy::from_path, hiding failures; update load_env_file to inspect the Result from dotenvy::from_path(&env_file) and, on Err, emit a tracing::warn! (including the error and the env_file path/metadata) so operators see malformed or permission errors while keeping behavior otherwise the same; reference the existing load_env_file function and the call to dotenvy::from_path to locate where to add the Result handling and tracing::warn! (prefer tracing spans if you already have one in scope, otherwise a simple tracing::warn! with the error and file path is fine).
138-261: Test suite LGTM; one optional hardening note.Tests comprehensively cover precedence (env > toml), blank/whitespace fallback, trimming, and missing-field TOML parsing. One small resilience nit:
ENV_LOCK.lock().unwrap()will poison the mutex if any test in the critical section panics, cascading failures into subsequent tests. Usinglock().unwrap_or_else(|e| e.into_inner())makes the suite more robust to a single failure.As per coding guidelines: "Co-locate tests in their modules under
mod testsblocks with descriptive names" — the descriptive test names here comply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/util.rs` around lines 138 - 261, Tests currently call ENV_LOCK.lock().unwrap(), which will poison the mutex and cause cascading failures if any test panics; change those calls to use lock().unwrap_or_else(|e| e.into_inner()) so the guard is recovered instead of panicking. Update the five test functions that create the lock (_lock in env_var_overrides_toml_nsec, empty_env_var_falls_back_to_toml, no_env_var_keeps_toml, whitespace_only_env_is_ignored, env_var_value_is_trimmed) to acquire the mutex with ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()).
40-60: Validate non-emptynsec_privkeyafter the env override.With
nsec_privkeynow#[serde(default)]and the env override applied here, it's possible forsettings.nostr.nsec_privkeyto be an empty string (no TOML value AND no/whitespace-only env var) wheninit_mostro_settingsruns. The failure currently surfaces later insrc/util.rs::get_keysas a generic "Failed to parse nostr private key" message. A startup-time check gives operators a clearer, more actionable error and fails fast.🛡️ Proposed fix
/// Validates Mostro settings on startup fn validate_mostro_settings(settings: &Settings) -> Result<(), MostroError> { let dev_fee = settings.mostro.dev_fee_percentage; @@ + if settings.nostr.nsec_privkey.trim().is_empty() { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "Nostr private key is missing. Set `{}` in the environment (or `{}/.env`), or set `nsec_privkey` in settings.toml.", + NSEC_ENV_VAR, ENV_FILENAME + )))); + } + Ok(()) }Also applies to: 105-106, 122-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/util.rs` around lines 40 - 60, Add a startup validation in validate_mostro_settings to ensure settings.nostr.nsec_privkey is not empty or whitespace after env overrides: check settings.nostr.nsec_privkey.trim().is_empty() and return an immediate MostroInternalErr/ServiceError with a clear message like "nostr private key (nsec_privkey) is empty after env override" so init_mostro_settings fails fast (this prevents the later generic error in get_keys).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 523-526: The fenced code block in README.md showing the
MOSTRO_NSEC_PRIVKEY example is missing a language specifier and triggers
markdownlint MD040; update the triple-backtick that opens the block to include a
language (e.g., "ini") so the block becomes ```ini and thus documents syntax and
satisfies MD040—look for the snippet containing MOSTRO_NSEC_PRIVKEY and change
the opening fence accordingly.
In `@src/config/wizard.rs`:
- Around line 200-206: The formatting mismatch comes from the first println!
splitting a single string literal across multiple lines; update that println!
(the one starting with "Storing your nsec as an environment variable (instead of
settings.toml) keeps") so it uses a single contiguous string literal like the
subsequent println! calls (i.e., combine the wrapped lines into one println!
call with the full message) so rustfmt/cargo fmt will no longer reflow it and CI
will pass.
---
Nitpick comments:
In `@src/config/util.rs`:
- Around line 21-26: The function load_env_file currently discards the result of
dotenvy::from_path, hiding failures; update load_env_file to inspect the Result
from dotenvy::from_path(&env_file) and, on Err, emit a tracing::warn! (including
the error and the env_file path/metadata) so operators see malformed or
permission errors while keeping behavior otherwise the same; reference the
existing load_env_file function and the call to dotenvy::from_path to locate
where to add the Result handling and tracing::warn! (prefer tracing spans if you
already have one in scope, otherwise a simple tracing::warn! with the error and
file path is fine).
- Around line 138-261: Tests currently call ENV_LOCK.lock().unwrap(), which will
poison the mutex and cause cascading failures if any test panics; change those
calls to use lock().unwrap_or_else(|e| e.into_inner()) so the guard is recovered
instead of panicking. Update the five test functions that create the lock (_lock
in env_var_overrides_toml_nsec, empty_env_var_falls_back_to_toml,
no_env_var_keeps_toml, whitespace_only_env_is_ignored, env_var_value_is_trimmed)
to acquire the mutex with ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()).
- Around line 40-60: Add a startup validation in validate_mostro_settings to
ensure settings.nostr.nsec_privkey is not empty or whitespace after env
overrides: check settings.nostr.nsec_privkey.trim().is_empty() and return an
immediate MostroInternalErr/ServiceError with a clear message like "nostr
private key (nsec_privkey) is empty after env override" so init_mostro_settings
fails fast (this prevents the later generic error in get_keys).
In `@src/config/wizard.rs`:
- Line 208: Replace the hardcoded ".env" and "MOSTRO_NSEC_PRIVKEY" occurrences
with the shared constants ENV_FILENAME and NSEC_ENV_VAR from
crate::config::constants: update wizard.rs (where env_file_path is built and
where the NSEC env var is referenced) and util.rs to import
crate::config::constants::{ENV_FILENAME, NSEC_ENV_VAR} instead of defining or
hardcoding these values locally, and remove the duplicate local definitions so
both modules reuse the single source of truth.
- Around line 247-270: In write_env_file, ensure file permissions are enforced
after opening by calling std::fs::set_permissions(path,
Permissions::from_mode(0o600)) (or the cross-platform equivalent) so existing
files get 0o600 even when not created; and avoid blind truncate by implementing
an "append-or-replace" flow: if the file exists, read its contents, replace any
line starting with "MOSTRO_NSEC_PRIVKEY=" with the new value (or append the line
if missing), then write the updated content back (you may truncate when writing
the final assembled content). Keep these changes localized to the write_env_file
function so it still uses OpenOptions for creation but then enforces permissions
and safely updates the MOSTRO_NSEC_PRIVKEY entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7d2c87c-cdb5-431e-9b65-65c80b70aad1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlREADME.mddocs/STARTUP_AND_CONFIG.mdsrc/config/types.rssrc/config/util.rssrc/config/wizard.rs
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I did a strict pass and I can't approve this PR yet.
The overall direction makes sense and I agree with merging env-based support first as the backward-compatible transition step. CI is green, the scope is coherent, and the precedence behavior is well covered by tests.
However, I see two issues that are worth fixing before approval:
write_env_filedoes not actually guarantee0600on an existing.envfile.
On Unix, OpenOptionsExt::mode(0o600) only applies when the file is created. If ~/.mostro/.env already exists with broader permissions, this code truncates and rewrites it but keeps the old mode bits. That defeats the security claim printed by the wizard.
At minimum, the function should explicitly set_permissions(..., 0o600) after opening/writing on Unix.
.envloading failures are silently ignored.
load_env_file currently discards the result of dotenvy::from_path. If the operator relies on <settings_dir>/.env and that file is malformed or unreadable, startup quietly falls back to TOML or to an empty key, and the eventual failure becomes much harder to diagnose.
This should at least emit a warning with the file path and the parsing/loading error.
These are not conceptual blockers to the feature itself, but they are real robustness/security gaps in the current implementation. Once they are addressed, I think this PR is in good shape for merge as the compatibility-preserving first step before the later file-based secret migration/deprecation work.
|
I do understand how using an environment variable improves security wise compared to nsec inline in config. However, I am uneasy with the .env file support and its inclusion in the setup wizard. Once there is .env support that includes secrets, we would be back at that place where there is a risk to leak for instance when versioning production setup. |
|
Hi @gotcha , fair point. I pushed a small change to the wizard so the two storage options are presented neutrally, without the "recommended" / "legacy" labels. The preamble also mentions that setting MOSTRO_NSEC_PRIVKEY via systemd, Docker or a secrets manager is a valid path, so nobody is nudged into a .env file if they don't want one. The intent here is purely additive. We're not deprecating inline nsec_privkey or asking anyone to migrate. Operators who want stronger isolation can opt in, the rest keep working unchanged. The file-path option you described fits naturally on top of this and can land in a follow-up PR whenever there's appetite. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Strict review pass after the latest update. I would not approve this yet. The two issues from my previous review that blocked approval are fixed, nice catch there:
However, I still see one thing worth cleaning up before approval:
The wizard still hardcodes both That is not a runtime bug today, but it is a maintainability problem in exactly the place where secret-handling behavior is being introduced. If the variable name or env filename ever changes, this code is now vulnerable to drift between the startup path and the wizard path. I would fix that by moving both constants to a shared config constants module and reusing them from both Once that duplication is removed, I would be comfortable approving this PR. |
Move ENV_FILENAME and NSEC_ENV_VAR from config/util.rs to config/constants.rs to share them between the wizard and loader modules, eliminating duplicate string literals.
There was a problem hiding this comment.
I did another strict pass after the latest changes.
The remaining maintainability concern is resolved: ENV_FILENAME and NSEC_ENV_VAR are now centralized in src/config/constants.rs and reused from both the loader and the wizard, so the env-loading path and the setup path can no longer drift.
The earlier blockers are also fixed:
.envload failures are surfaced with a warning instead of being silently ignored.write_env_file()now tightens permissions on pre-existing files instead of only relying onOpenOptionsExt::mode(0o600).
At this point I am comfortable approving this as a backward-compatible first step for env-based secret handling. The scope is coherent, existing inline settings.toml setups keep working, and the precedence behavior is covered by tests.
Approved.
3 participants
Motivation
Storing the Nostr private key inline in
settings.tomlmakes it easy to leak it accidentally: config files end up in logs, backups, bug reports, and chat messages when operators ask for help. It also makes integration with Docker secrets, systemd credentials, and similar tools harder than it should be.This PR lets operators provide the nsec via the
MOSTRO_NSEC_PRIVKEYenvironment variable, which is the dominant pattern across Nostr daemons (Alby Hub, NWC, OpenClaw) and follows 12-factor conventions.Changes
MOSTRO_NSEC_PRIVKEYenv var support. Precedence: env var ><settings_dir>/.env(auto-loaded viadotenvy) >settings.toml.nsec_privkeyoptional insettings.tomlvia#[serde(default)], sooperators relying on the env var can omit the field entirely.
~/.mostro/.env(recommended,chmod 600) or inline insettings.toml.whitespace-only env ignored, no env keeps TOML, trimming, TOML without the
field).
Backward compatibility
Fully backward-compatible. Existing
nsec_privkey = 'nsec1...'insettings.tomlkeeps working exactly as before. The env var only takes effect when explicitly set.Summary by CodeRabbit
New Features
Documentation