refactor: load Nostr private key from file instead of config#704
Open
gotcha wants to merge 1 commit intoMostroP2P:mainfrom
Open
refactor: load Nostr private key from file instead of config#704gotcha wants to merge 1 commit intoMostroP2P:mainfrom
gotcha wants to merge 1 commit intoMostroP2P:mainfrom
Conversation
Contributor
WalkthroughSwitch Nostr private-key config from inline to a file path, make key loading async, persist generated nsec to disk in the setup wizard, and change tag/event APIs to accept a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Wizard as SetupWizard
participant FS as Filesystem
participant App as MostroApp
participant Keys as KeyLoader
participant NostrLib as Nostr
User->>Wizard: run_setup_wizard(settings_dir)
Wizard->>User: prompt for/generate nsec
Wizard->>FS: write settings_dir/nostr-key.nsec (0o600)
FS-->>Wizard: write OK
Wizard->>App: return NostrSettings(nsec_privkey_file)
%% Runtime flow
App->>Keys: get_keys().await (reads nsec file)
Keys->>FS: read nsec file
FS-->>Keys: nsec bytes
Keys->>NostrLib: parse Keys from nsec
NostrLib-->>Keys: Keys (private/public)
App->>NostrLib: create event using &PublicKey
NostrLib-->>App: signed event / tags
App->>NostrLib: publish to relays
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/util.rs (1)
311-311: Avoid redundant async key loading.These lines call
get_keys().await?to get the public key, but the keys are already available from the function parameters:
- Lines 311, 317:
mostro_keysparameter is passed toget_tags_for_new_order- Line 756:
keysparameter is passed toupdate_order_eventEach
get_keys().awaitre-reads the key file from disk, which is unnecessary I/O overhead.♻️ Proposed fix for get_tags_for_new_order (lines 311, 317)
match is_user_present(pool, identity_pubkey.to_string()).await { Ok(user) => { // We transform the order fields to tags to use in the event order_to_tags( new_order_db, Some((user.total_rating, user.total_reviews, user.created_at)), Some(&mostro_pubkey), - &get_keys().await?.public_key(), + &mostro_keys.public_key(), ) } Err(_) => { // We transform the order fields to tags to use in the event if identity_pubkey == trade_pubkey { - order_to_tags(new_order_db, Some((0.0, 0, 0)), Some(&mostro_pubkey), &get_keys().await?.public_key()) + order_to_tags(new_order_db, Some((0.0, 0, 0)), Some(&mostro_pubkey), &mostro_keys.public_key()) } else { Err(MostroInternalErr(ServiceError::InvalidPubkey)) } } }♻️ Proposed fix for update_order_event (line 756)
let mostro_pubkey = keys.public_key().to_hex(); - if let Some(tags) = order_to_tags(&order_updated, reputation_data, Some(&mostro_pubkey), &get_keys().await?.public_key())? { + if let Some(tags) = order_to_tags(&order_updated, reputation_data, Some(&mostro_pubkey), &keys.public_key())? {Also applies to: 317-317, 756-756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util.rs` at line 311, The code redundantly calls get_keys().await? to read keys from disk even though the functions already receive keys as parameters; replace the get_keys().await?.public_key() (and any other get_keys().await? usages) inside get_tags_for_new_order with the passed-in mostro_keys.public_key() (or equivalent accessor on the mostro_keys parameter), and in update_order_event replace the get_keys().await? usage with the provided keys.public_key() (or the appropriate field/method on the keys parameter), ensuring you remove the extra awaits and error propagation and use the existing parameters instead.src/app/release.rs (1)
687-689: LGTM!Both branches correctly pass
&ctx.keys().public_key()as the newdefault_pubkeyparameter toorder_to_tags().Consider breaking line 689 for readability:
✨ Optional: improve line length
- Err(_) => order_to_tags(new_order, Some((0.0, 0, 0)), Some(&mostro_pubkey), &ctx.keys().public_key())?, + Err(_) => order_to_tags( + new_order, + Some((0.0, 0, 0)), + Some(&mostro_pubkey), + &ctx.keys().public_key(), + )?,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/release.rs` around lines 687 - 689, The call to order_to_tags in the Err branch is long; split the argument list across multiple lines to improve readability—e.g. put each argument on its own line or break before the final parameter so the call (order_to_tags(new_order, Some((0.0, 0, 0)), Some(&mostro_pubkey), &ctx.keys().public_key())?) becomes a multi-line invocation; update the Err arm containing order_to_tags, referencing new_order, mostro_pubkey, and ctx.keys().public_key() to maintain the same arguments and trailing ?.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/types.rs`:
- Around line 128-129: The doc comment for the struct field nsec_privkey_file is
outdated; update the comment to state that this field stores the file path to
the Nostr private key (e.g., "Path to file containing the Nostr private key
(nsec)") instead of saying "Nostr private key", so readers know it holds a file
path rather than the key material itself.
- Around line 126-132: The NostrSettings field was renamed to nsec_privkey_file
but other code still references nsec_privkey; update all references to use the
new field name (nsec_privkey_file). Specifically, change struct initializers
that set NostrSettings::{nsec_privkey => nsec_privkey_file} (e.g., where
NostrSettings is constructed in the wizard/TestContextBuilder) and update all
property accesses like settings.nostr.nsec_privkey and
nostr_settings.nostr.nsec_privkey to settings.nostr.nsec_privkey_file /
nostr_settings.nostr.nsec_privkey_file so the field name matches the
NostrSettings definition.
In `@src/util.rs`:
- Line 664: The tracing::error call in util.rs currently logs the sensitive
variable nsec_privkey; remove the private key from logs and only log
non-sensitive context and the error. Replace the tracing::error invocation that
references nsec_privkey (the tracing::error!("Failed to parse nostr private key:
{} {}", e, nsec_privkey) call) with a message that omits or redacts nsec_privkey
(e.g., log the error and a generic note like "private key redacted" or a
non-sensitive identifier), ensuring no secret data (nsec_privkey) is included in
logs.
- Around line 640-655: The function load_nostr_private_key currently logs the
wrong failure message and calls .unwrap() on String::from_utf8 which can panic;
change the read error log/message to "Failed to read nostr private key file" and
include the read error and path, then avoid unwrap by converting bytes -> String
using String::from_utf8(...) and map the Utf8Error into a
MostroInternalErr(ServiceError::NostrError(...)) that includes the path and
error; use ? to propagate both mapped errors and finally trim().to_string() to
return the key. Keep references: load_nostr_private_key, tokio::fs::read,
String::from_utf8, MostroInternalErr, ServiceError::NostrError, and
path.display().
---
Nitpick comments:
In `@src/app/release.rs`:
- Around line 687-689: The call to order_to_tags in the Err branch is long;
split the argument list across multiple lines to improve readability—e.g. put
each argument on its own line or break before the final parameter so the call
(order_to_tags(new_order, Some((0.0, 0, 0)), Some(&mostro_pubkey),
&ctx.keys().public_key())?) becomes a multi-line invocation; update the Err arm
containing order_to_tags, referencing new_order, mostro_pubkey, and
ctx.keys().public_key() to maintain the same arguments and trailing ?.
In `@src/util.rs`:
- Line 311: The code redundantly calls get_keys().await? to read keys from disk
even though the functions already receive keys as parameters; replace the
get_keys().await?.public_key() (and any other get_keys().await? usages) inside
get_tags_for_new_order with the passed-in mostro_keys.public_key() (or
equivalent accessor on the mostro_keys parameter), and in update_order_event
replace the get_keys().await? usage with the provided keys.public_key() (or the
appropriate field/method on the keys parameter), ensuring you remove the extra
awaits and error propagation and use the existing parameters instead.
🪄 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: 231725bd-24e8-43dc-9aa4-b8458734570d
📒 Files selected for processing (8)
.gitignoresettings.tpl.tomlsrc/app/release.rssrc/bitcoin_price.rssrc/config/types.rssrc/main.rssrc/nip33.rssrc/util.rs
jfroche
force-pushed
the
nsec-privkey-file
branch
from
05273bc to
02f52fd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/util.rs (1)
661-672: Cache parsedKeysinstead of reloading the secret file per call.
get_keys()now hits the filesystem every time it's called. That adds avoidable I/O to hot paths like invoice handling and makes those flows fail if the key file becomes unreadable after startup, even though the process already parsed keys elsewhere. Prefer cachingKeysonce and threading/cloning them from there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util.rs` around lines 661 - 672, get_keys() currently reads the nsec file and reparses Keys on every call; change it to parse once at startup (or first call) and cache the parsed Keys in a shared, cloneable container (e.g. a module-level OnceCell/OnceLock or lazy static holding an Arc<Keys> or Keys wrapped for cheap cloning) so subsequent calls to get_keys() return the cached Keys (cloning/Arc::clone) instead of calling load_nostr_private_key and Keys::parse repeatedly; keep Settings::get_nostr() usage for initial load path and ensure errors on initial parse still return MostroError as before.src/app/context.rs (1)
263-272: Use a unique temp file for the test NSEC fixture.
std::env::temp_dir().join("mostro-test-nostr-key.nsec")is shared across all test processes. Parallel runs can clobber each other or pick up stale contents. Prefer a per-test unique filename here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/context.rs` around lines 263 - 272, The test currently writes the NSEC fixture to a fixed path (nsec_path) which can clash across parallel runs; change the setup that creates nsec_path so it uses a unique temporary file per test (e.g. tempfile::NamedTempFile or tempfile::Builder with a random suffix) instead of std::env::temp_dir().join("mostro-test-nostr-key.nsec"), write nsec_key into that unique temp file, and pass its path into Settings -> NostrSettings via nsec_privkey_file (ensure the temp file lives long enough for the test or persist its path appropriately).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/wizard.rs`:
- Around line 177-190: The code unconditionally imports and uses
std::os::unix::fs::OpenOptionsExt when creating nsec_privkey_file which fails on
non-Unix targets; wrap the Unix-specific import and the OpenOptions::mode(0o600)
usage in #[cfg(unix)] and provide a #[cfg(not(unix))] alternative that
opens/creates/truncates the file without calling .mode(…) (e.g. use
std::fs::OpenOptions::new().write(true).create(true).truncate(true).open(&nsec_privkey_file)),
keeping the same error mapping and file.write_all(nsec.as_bytes()) logic for
both branches so nsec_privkey_file, the OpenOptions usage, and error handling
remain consistent across platforms.
In `@src/nip33.rs`:
- Around line 356-360: Update the /// doc comment for the public function
order_to_tags to reflect the new signature: document that mostro_pubkey is now
required and must be passed as &nostr::PublicKey (no longer optional or derived
via get_keys()), and adjust any examples/parameter descriptions to remove
references to optional behavior or get_keys() and explain expected semantics of
reputation_data and the Result<Option<Tags>, MostroError> return; update the
parameter doc lines to name the exact parameter mostro_pubkey and describe its
purpose and ownership/borrowing expectations.
---
Nitpick comments:
In `@src/app/context.rs`:
- Around line 263-272: The test currently writes the NSEC fixture to a fixed
path (nsec_path) which can clash across parallel runs; change the setup that
creates nsec_path so it uses a unique temporary file per test (e.g.
tempfile::NamedTempFile or tempfile::Builder with a random suffix) instead of
std::env::temp_dir().join("mostro-test-nostr-key.nsec"), write nsec_key into
that unique temp file, and pass its path into Settings -> NostrSettings via
nsec_privkey_file (ensure the temp file lives long enough for the test or
persist its path appropriately).
In `@src/util.rs`:
- Around line 661-672: get_keys() currently reads the nsec file and reparses
Keys on every call; change it to parse once at startup (or first call) and cache
the parsed Keys in a shared, cloneable container (e.g. a module-level
OnceCell/OnceLock or lazy static holding an Arc<Keys> or Keys wrapped for cheap
cloning) so subsequent calls to get_keys() return the cached Keys
(cloning/Arc::clone) instead of calling load_nostr_private_key and Keys::parse
repeatedly; keep Settings::get_nostr() usage for initial load path and ensure
errors on initial parse still return MostroError as before.
🪄 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: 8775f1c9-bfec-4623-8cbc-da08cc9f37c9
📒 Files selected for processing (11)
.gitignoresettings.tpl.tomlsrc/app/context.rssrc/app/release.rssrc/bitcoin_price.rssrc/config/mod.rssrc/config/types.rssrc/config/wizard.rssrc/main.rssrc/nip33.rssrc/util.rs
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- settings.tpl.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main.rs
- src/bitcoin_price.rs
- src/config/types.rs
It is more secure to put secrets in their own files, in this case, the nostr private key. - Change nsec_privkey to nsec_privkey_file in settings - Add async load_nostr_private_key() helper function that loads private key from file - Make get_keys() async and use load_nostr_private_key() - Update all get_keys() call sites to await - Add default_pubkey parameter to order_to_tags() to avoid making it async - Add vendor to .gitignore
jfroche
force-pushed
the
nsec-privkey-file
branch
from
02f52fd to
cbc6d62
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
README.md (1)
512-514: Mention restrictive permissions for thensecfile.The new flow moves the secret out of
settings.toml, but the docs still don't tell manual installs to keep that file owner-readable only. A shortchmod 600note here would prevent accidental exposure.Suggested doc addition
Save the generated `nsec` in the file referenced by `nsec_privkey_file`. Mostro expects that file to contain only the `nsec` value. +Restrict the file to the Mostro user only (for example, `chmod 600 ~/.mostro/nostr-key.nsec`). Inline `nostr.nsec_privkey` is no longer supported and startup will fail until you move the key into a file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 512 - 514, Add a short note to the README where you describe saving the generated nsec to the file referenced by nsec_privkey_file: state that the file should be owner-readable only and show the permission recommendation (e.g., chmod 600) so manual installers restrict access; reference the nsec value placement (moving it out of nostr.nsec_privkey) and explain that storing the key in a file requires setting restrictive permissions to avoid accidental exposure.src/util.rs (1)
661-677: Document the new async/file-backedget_keyscontract.This public API now performs file I/O, depends on
settings.nostr.nsec_privkey_file, and fails on missing/invalid file contents. Please add///docs so callers know it is no longer a cheap in-memory lookup.As per coding guidelines, "Document non-obvious public APIs with
///doc comments".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util.rs` around lines 661 - 677, Add a Rust doc comment (///) above the public async function get_keys to document that it performs async file I/O, relies on Settings::get_nostr().nsec_privkey_file, can fail if the file is missing or contains an invalid private key, and returns Result<Keys, MostroError>. Mention that it calls load_nostr_private_key and Keys::parse, that callers should await it and handle MostroInternalErr(ServiceError::NostrError) for missing/invalid keys, and note any side effects (reading disk) so callers know it is not a cheap in-memory lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@settings.tpl.toml`:
- Line 22: Update the template default path for the private key so it matches
the runtime/default location used by run_setup_wizard(): replace the hardcoded
'/home/user/nostr-key.nsec' value for the nsec_privkey_file entry in
settings.tpl.toml with the Mostro settings dir path (~/.mostro/nostr-key.nsec)
so the copied settings (when creating ~/.mostro/settings.toml) and documentation
are consistent with the actual runtime behavior.
In `@src/app/context.rs`:
- Around line 266-276: The test currently embeds a real-looking Nostr private
key in the nsec_key variable which triggers secret scanners; replace the
hard-coded string by generating a throwaway/test key at runtime (or load a
scanner-excluded fixture) instead: in the test setup where nsec_key is defined,
call a local helper or library function to produce a temporary/ephemeral nsec
value and continue to use TEST_KEY_FILE_COUNTER, nsec_dir and nsec_path as
before so the rest of the test is unchanged; ensure the generated key matches
the expected nsec format used by the code under test so writes to nsec_path
succeed without exposing any real secret.
In `@src/config/mod.rs`:
- Around line 207-220: Replace the real-looking nsec string used for TOML
deserialization with an obvious dummy placeholder in the test function
test_nostr_settings_legacy_inline_key (replace the value assigned to
legacy_settings where nsec_privkey is set) and update the corresponding literal
in src/config/util.rs so both places use the same clearly fake placeholder;
ensure the test still asserts the same fields
(nostr_settings.nostr.nsec_privkey_file, nostr_settings.nostr.nsec_privkey, and
nostr_settings.nostr.relays) but with the new placeholder value instead of the
long real-looking nsec string.
---
Nitpick comments:
In `@README.md`:
- Around line 512-514: Add a short note to the README where you describe saving
the generated nsec to the file referenced by nsec_privkey_file: state that the
file should be owner-readable only and show the permission recommendation (e.g.,
chmod 600) so manual installers restrict access; reference the nsec value
placement (moving it out of nostr.nsec_privkey) and explain that storing the key
in a file requires setting restrictive permissions to avoid accidental exposure.
In `@src/util.rs`:
- Around line 661-677: Add a Rust doc comment (///) above the public async
function get_keys to document that it performs async file I/O, relies on
Settings::get_nostr().nsec_privkey_file, can fail if the file is missing or
contains an invalid private key, and returns Result<Keys, MostroError>. Mention
that it calls load_nostr_private_key and Keys::parse, that callers should await
it and handle MostroInternalErr(ServiceError::NostrError) for missing/invalid
keys, and note any side effects (reading disk) so callers know it is not a cheap
in-memory lookup.
🪄 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: b09f2806-8da6-4518-b15c-0c34d437e21a
📒 Files selected for processing (15)
.codex.gitignoreREADME.mddocs/STARTUP_AND_CONFIG.mdsettings.tpl.tomlsrc/app/context.rssrc/app/release.rssrc/bitcoin_price.rssrc/config/mod.rssrc/config/types.rssrc/config/util.rssrc/config/wizard.rssrc/main.rssrc/nip33.rssrc/util.rs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.rs
- src/config/types.rs
|
|
||
| [nostr] | ||
| nsec_privkey = 'nsec1...' | ||
| nsec_privkey_file = '/home/user/nostr-key.nsec' |
Contributor
There was a problem hiding this comment.
Keep the template path aligned with the runtime default location.
run_setup_wizard() now writes the key under the Mostro settings dir, and the docs/examples point to ~/.mostro/nostr-key.nsec. Leaving /home/user/nostr-key.nsec here makes the copied template inconsistent and easy to misconfigure.
Suggested tweak
-nsec_privkey_file = '/home/user/nostr-key.nsec'
+nsec_privkey_file = '/home/user/.mostro/nostr-key.nsec'Based on learnings, copy from settings.tpl.toml to ~/.mostro/settings.toml for local runs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| nsec_privkey_file = '/home/user/nostr-key.nsec' | |
| nsec_privkey_file = '/home/user/.mostro/nostr-key.nsec' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@settings.tpl.toml` at line 22, Update the template default path for the
private key so it matches the runtime/default location used by
run_setup_wizard(): replace the hardcoded '/home/user/nostr-key.nsec' value for
the nsec_privkey_file entry in settings.tpl.toml with the Mostro settings dir
path (~/.mostro/nostr-key.nsec) so the copied settings (when creating
~/.mostro/settings.toml) and documentation are consistent with the actual
runtime behavior.
Comment on lines
+266
to
+276
| let nsec_key = "nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd"; | ||
| let counter = TEST_KEY_FILE_COUNTER.fetch_add(1, Ordering::Relaxed); | ||
| let nsec_dir = std::env::temp_dir().join(format!( | ||
| "mostro-test-{}-{}", | ||
| std::process::id(), | ||
| counter | ||
| )); | ||
| std::fs::create_dir_all(&nsec_dir).expect("failed to create test nsec key directory"); | ||
| let nsec_path = nsec_dir.join("nostr-key.nsec"); | ||
| std::fs::write(&nsec_path, nsec_key).expect("failed to write test nsec key file"); | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid committing a valid-looking nsec test secret.
Line 266 embeds a real-looking Nostr private key, and secret scanners will keep flagging it even if it is only test data. Prefer generating a throwaway key at test setup time or loading a scanner-excluded fixture so this path stops producing false-positive secret incidents.
🧰 Tools
🪛 Betterleaks (1.1.1)
[high] 266-266: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/context.rs` around lines 266 - 276, The test currently embeds a
real-looking Nostr private key in the nsec_key variable which triggers secret
scanners; replace the hard-coded string by generating a throwaway/test key at
runtime (or load a scanner-excluded fixture) instead: in the test setup where
nsec_key is defined, call a local helper or library function to produce a
temporary/ephemeral nsec value and continue to use TEST_KEY_FILE_COUNTER,
nsec_dir and nsec_path as before so the rest of the test is unchanged; ensure
the generated key matches the expected nsec format used by the code under test
so writes to nsec_path succeed without exposing any real secret.
Comment on lines
+207
to
+220
| #[test] | ||
| fn test_nostr_settings_legacy_inline_key() { | ||
| let legacy_settings = r#"[nostr] | ||
| nsec_privkey = 'nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd' | ||
| relays = ['wss://relay.damus.io']"#; | ||
| let nostr_settings: StubSettingsNostr = | ||
| toml::from_str(legacy_settings).expect("Failed to deserialize"); | ||
|
|
||
| assert_eq!(nostr_settings.nostr.nsec_privkey_file, ""); | ||
| assert_eq!( | ||
| nostr_settings.nostr.nsec_privkey.as_deref(), | ||
| Some("nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd") | ||
| ); | ||
| assert_eq!(nostr_settings.nostr.relays, vec!["wss://relay.damus.io"]); |
Contributor
There was a problem hiding this comment.
Use an obvious dummy value instead of a real-looking nsec fixture.
This test only exercises TOML deserialization, so the long nsec... string just trips secret scanners. Betterleaks is already flagging Line 210. Please swap this and the matching new literal in src/config/util.rs for a clearly fake placeholder.
Suggested tweak
- nsec_privkey = 'nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd'
+ nsec_privkey = 'legacy-inline-key-for-test-only'
...
- Some("nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd")
+ Some("legacy-inline-key-for-test-only")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn test_nostr_settings_legacy_inline_key() { | |
| let legacy_settings = r#"[nostr] | |
| nsec_privkey = 'nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd' | |
| relays = ['wss://relay.damus.io']"#; | |
| let nostr_settings: StubSettingsNostr = | |
| toml::from_str(legacy_settings).expect("Failed to deserialize"); | |
| assert_eq!(nostr_settings.nostr.nsec_privkey_file, ""); | |
| assert_eq!( | |
| nostr_settings.nostr.nsec_privkey.as_deref(), | |
| Some("nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd") | |
| ); | |
| assert_eq!(nostr_settings.nostr.relays, vec!["wss://relay.damus.io"]); | |
| #[test] | |
| fn test_nostr_settings_legacy_inline_key() { | |
| let legacy_settings = r#"[nostr] | |
| nsec_privkey = 'legacy-inline-key-for-test-only' | |
| relays = ['wss://relay.damus.io']"#; | |
| let nostr_settings: StubSettingsNostr = | |
| toml::from_str(legacy_settings).expect("Failed to deserialize"); | |
| assert_eq!(nostr_settings.nostr.nsec_privkey_file, ""); | |
| assert_eq!( | |
| nostr_settings.nostr.nsec_privkey.as_deref(), | |
| Some("legacy-inline-key-for-test-only") | |
| ); | |
| assert_eq!(nostr_settings.nostr.relays, vec!["wss://relay.damus.io"]); |
🧰 Tools
🪛 Betterleaks (1.1.1)
[high] 210-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/mod.rs` around lines 207 - 220, Replace the real-looking nsec
string used for TOML deserialization with an obvious dummy placeholder in the
test function test_nostr_settings_legacy_inline_key (replace the value assigned
to legacy_settings where nsec_privkey is set) and update the corresponding
literal in src/config/util.rs so both places use the same clearly fake
placeholder; ensure the test still asserts the same fields
(nostr_settings.nostr.nsec_privkey_file, nostr_settings.nostr.nsec_privkey, and
nostr_settings.nostr.relays) but with the new placeholder value instead of the
long real-looking nsec string.
Collaborator
|
Thanks for the PR @gotcha ! Always welcome, please fix the request of @coderabbitai to complete the work, than we will evaluate the pr! |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 26004
🔴 Actionable (must-fix)
🟡 Nitpick (nice-to-have)
🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It is more secure to put secrets in their own files, in this case, the nostr private key.
Summary by CodeRabbit
New Features
Chores
Documentation
Tests