fix: scrub Nostr keys from 14 more log lines + add a CI check (#836) - #842
Open
ToRyVand wants to merge 5 commits into
Open
fix: scrub Nostr keys from 14 more log lines + add a CI check (#836)#842ToRyVand wants to merge 5 commits into
ToRyVand wants to merge 5 commits into
Conversation
…P2P#836) AGENTS.md:48 says to scrub logs that might leak invoices or Nostr keys. MostroP2P#834/MostroP2P#835 fixed 3 instances in restore_session.rs; a follow-up review found 5 more scattered across the daemon (issue MostroP2P#836) but no mechanism to stop the pattern from recurring. Adds scripts/check_log_redaction.py, wired into ci.yml, which fails the build on any tracing::{trace,debug,info,warn,error}! call that interpolates a Nostr key/identity-shaped argument. Running it against the current codebase surfaced 9 more instances beyond the 5 already documented, including util::send_dm logging both sender and receiver on every outbound protocol message. Fixes all 14 with the same one-line-per-site treatment MostroP2P#834/MostroP2P#835 used, so the new check ships green. cancel.rs: removing taker_pubkey from one log line left the parameter fully unused across cancel_order_by_taker_inner and its only caller, cancel_order_by_taker — dropped from both signatures and their call sites rather than silenced.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request adds a Python CI gate that detects key-like identifiers in Rust tracing calls, removes sensitive values from affected logs, updates cancellation function signatures, and requires the redaction check before tests run. ChangesLog redaction enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant LogRedactionTests
participant LogRedactionChecker
participant RustSource
CI->>LogRedactionTests: Run regression tests
LogRedactionTests->>LogRedactionChecker: Check temporary Rust snippets
CI->>LogRedactionChecker: Scan src/**/*.rs
LogRedactionChecker->>RustSource: Inspect tracing macro calls
LogRedactionChecker-->>CI: Return success or violations
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_log_redaction.py`:
- Around line 25-38: Update SUSPICIOUS_RE and the send_dm logging path to
prevent cleartext serialized identities or invoices from bypassing redaction
checks: cover identity_key and sender_key variants, add detection for opaque
payload/message content where feasible, and remove or redact payload logs that
cannot be reliably identified by names. Keep the existing narrowly targeted
key-name matching without broadening it to generic key variables.
- Line 23: Extend the scanner around MACRO_RE and its span-parsing logic to
recognize Rust macro calls with parentheses, braces, and angle brackets,
including whitespace before delimiters. Make tokenization/span detection
Rust-aware so comments and string syntax cannot prematurely terminate or skip
macro arguments, and add regression coverage for every supported delimiter and
edge case before using the scanner as a security gate.
In `@src/util.rs`:
- Around line 707-711: Update the logging call in the surrounding
message-sending function to remove the serialized payload from the log entirely.
Retain only the event ID or safe action metadata, ensuring no payload fields
such as identities or invoice data are written.
🪄 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 Plus
Run ID: 11c5e720-fbd8-42c1-9bfd-170af7582526
📒 Files selected for processing (11)
.github/workflows/ci.ymlscripts/check_log_redaction.pysrc/app.rssrc/app/admin_take_dispute.rssrc/app/bond/payout.rssrc/app/cancel.rssrc/app/last_trade_index.rssrc/db.rssrc/rpc/service.rssrc/scheduler.rssrc/util.rs
admin_take_dispute_action sends a Payload::Peer{pubkey} to both parties
via send_dm; the trailing info! logged that payload in full, leaking
the solver's Nostr pubkey (AGENTS.md:48).
job_cancel_orders printed the full edited Order via println!, which carries buyer/seller/master pubkeys. tracing:: macros go through the new log-redaction CI gate; bare println! doesn't, so this slipped past it (AGENTS.md:48).
…y/sender_key variants
The scanner only matched name!(...) and bare identity/sender, missing
trace! {..}/trace![..] call forms and identity_key/sender_key-style
identifiers. Extend both, add regression tests for every delimiter
and identifier form, and wire the tests into the log-redaction CI job.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_log_redaction_test.py`:
- Around line 27-45: Update the positive cases in test_paren_call_flags_pubkey,
test_brace_call_flags_pubkey, test_bracket_call_flags_pubkey,
test_identity_key_variant_is_flagged, and test_sender_key_variant_is_flagged to
assert the exact reported violation tuples, including source line 1 and the
expected identifier ("pubkey", "identity_key", or "sender_key"), rather than
asserting only the violation count.
🪄 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 Plus
Run ID: f1e3764d-c601-4d83-bf15-1455847a82ba
📒 Files selected for processing (6)
.github/workflows/ci.yml.gitignorescripts/check_log_redaction.pyscripts/check_log_redaction_test.pysrc/scheduler.rssrc/util.rs
💤 Files with no reviewable changes (1)
- src/scheduler.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- src/util.rs
- scripts/check_log_redaction.py
The positive cases only checked the violation count, so the scanner could report the wrong key name or the wrong source line and every one of them would still pass. Assert the exact `(line, identifier)` tuples instead. The existing cases are all one-liners, so their line `1` would hold even if the number were never computed — add a case with the call further down the file so that arithmetic is actually exercised.
Contributor
Author
|
Addressed in The five positive cases now assert the exact self.assertEqual(violations, [(1, "pubkey")])
self.assertEqual(violations, [(1, "identity_key")])
self.assertEqual(violations, [(1, "sender_key")])One addition beyond the suggestion: every existing case is a one-liner, so asserting line def test_reported_line_is_the_macro_line_not_the_first(self):
violations = self._violations(
"fn x() {\n let a = 1;\n info!(\"{}\", pubkey);\n}"
)
self.assertEqual(violations, [(3, "pubkey")])Both CI steps pass locally: |
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
1 participant
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.
Summary
Closes #836.
AGENTS.md:48says to scrub logs that might leak invoices orNostr keys. #834/#835 fixed 3 instances in
restore_session.rs; a/code-reviewpass on that fix found 5 more scattered across the daemon,with no mechanism to stop the pattern from recurring — #836 asked for a
structural fix rather than another one-off patch.
Went with the lighter-weight of the two directions the issue proposed (a
CI check, vs. a
tracing_subscriber::Layerredacting at runtime): smaller,self-contained, faster to review. Trade-off: it only prevents new
instances at CI time, it doesn't redact anything at runtime.
scripts/check_log_redaction.py(new): flags anytracing::{trace,debug,info,warn,error}!(...)call whose argumentsinterpolate a Nostr key/identity-shaped identifier (
*pubkey*,identity,sender,master_key,trade_key,nsec*,priv(ate)?_?key*).Not a full Rust parser — it balances parens while blanking string-literal
contents (so a format string's own prose, e.g. "...taker pubkey in
order...", can't false-positive) and searches only the real arguments.
A
// pubkey-log-allow: <reason>comment on the line above a call exemptsa deliberate, documented exception.
.github/workflows/ci.yml: newlog-redactionjob, added totest'sneedsalongsidefmt/clippy.#836documented(
scheduler.rs,app.rs×2,last_trade_index.rs,db.rs) plus 9 thecheck itself found that manual review hadn't caught yet
(
admin_take_dispute.rs×2,bond/payout.rs×3,cancel.rs,rpc/service.rs,util.rs×2 — includingsend_dm, which logged bothsender and receiver on every single outbound protocol message, the
highest-frequency call site of this pattern in the daemon). Same
one-line-per-site treatment Nostr keys logged in cleartext in restore_session.rs (violates AGENTS.md log-scrubbing guideline) #834/fix(restore-session): scrub Nostr keys from log lines #835 used: drop the key, comment citing
AGENTS.md:48.cancel.rs: droppingtaker_pubkeyfrom one log line left the parameterfully unused in
cancel_order_by_taker_innerand its only caller,cancel_order_by_taker— removed from both signatures and their 2 callsites rather than silenced with an underscore.
Test plan
python3 scripts/check_log_redaction.py— clean against the final tree.cargo build— clean, no unused-variable warnings.cargo clippy --all-targets --all-features -- -D warnings— clean.cargo fmt --check— clean.cargo test— 1045 passed, 1 pre-existing unrelated flake(
lightning::invoice::tests::test_lnurl_validation_with_test_serverbinds a hardcoded
127.0.0.1:8080,AddrInUseon a busy port —unrelated to this diff).
Summary by CodeRabbit
Security & Privacy
Tests
CI