Skip to content

Add self-hosted Nostr DM notification provider#438

Merged
schjonhaug merged 19 commits into
masterfrom
schjonhaug/issue-417
Jun 16, 2026
Merged

Add self-hosted Nostr DM notification provider#438
schjonhaug merged 19 commits into
masterfrom
schjonhaug/issue-417

Conversation

@schjonhaug

Copy link
Copy Markdown
Owner

Summary

  • add self-hosted Nostr DM notification provider using nostr-sdk NIP-17 support
  • store contact Nostr recipients canonically as hex while displaying npub values
  • add generated instance sender key, Settings sender display, and test-send endpoint/UI
  • add contact and wallet notification UI support for Nostr delivery methods

Closes #417

Tests

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test --all-targets
  • pnpm run lint
  • pnpm test --runInBand
  • pnpm run build

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Your organization has disabled Claude subscription access for Claude Code · Use an Anthropic API key instead, or ask your admin to enable access

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Gemini Code Review

Here's a review of the provided changes in this pull request.

Code Quality and Best Practices

  • Robust Error Handling: Refactoring the has_method_changed closure to return a Result<bool, String> is an excellent improvement. It properly propagates validation errors from normalize_nostr_recipient_or_error instead of silently failing or using a dummy value.
  • Separation of Concerns: Using reject_nostr_in_cloud_mode keeps the handler logic clean and effectively centralizes the enforcement of self-hosted-only functionality.
  • SQLite Migrations: The approach taken in 035_add_nostr_notifications.sql to recreate the contact_notification_methods table using a temporary _new table is the correct and safest way to handle schema alterations (like updating a CHECK constraint) in SQLite.
  • Idempotency: Using INSERT OR IGNORE in 036_add_instance_settings.sql is a great practice, ensuring the migration can be safely rerun or applied over existing data without throwing a unique constraint violation.

Potential Bugs or Issues

  • Lost Triggers or Implicit Indexes: In 035_add_nostr_notifications.sql, the original table contact_notification_methods is dropped. While you correctly recreated the explicit indexes, verify if there were any database triggers attached to the old table. If there were, they need to be explicitly recreated as dropping the table drops its triggers as well.
  • instance_secrets Table Location: In migration 035, instance_secrets is created, but 036 creates a seemingly related instance_settings table. Make sure the secret table creation wasn't accidentally left in 035 instead of 036 or its own distinct migration file.
  • Truncated Response Construction: The diff cuts off in update_wallet_contact where the Err(e) branch returns a tuple into a response. Ensure that the constructed error (using ErrorResponse::coded) matches the established API error schema throughout the codebase.

Performance Considerations

  • Validation Overhead: The has_method_changed closure runs inside a loop and calls normalize_nostr_recipient_or_error. Assuming this involves Bech32 decoding (converting an npub to a hex public key), there is a slight CPU overhead. Given that users typically have very few notification methods per contact (e.g., 1-3), this O(N^2) check will be entirely negligible and perfectly fine for an I/O-bound web handler.

Security Concerns

  • Nostr Private Keys (Sender Identity): The updated .env.example.self-hosted file notes that the Nostr sender key is stored in the SQLite database and warns that anyone with DB access can impersonate the Canary instance. It's highly recommended to clearly document this in the official documentation, and if possible, ensure the instance_secrets table doesn't log its values to standard out or application logs during debug traces.
  • Target Validation: By forcing the use of normalize_nostr_recipient_or_error, you are successfully validating the target inputs before they ever touch the database, which mitigates the risk of bad actors storing excessively large strings or malformed payloads.

Test Coverage

  • From the list of changed files, it's great to see that migrations_tests.rs, contact_duplicates_test.rs, and frontend component tests (nostr-settings.test.tsx, notification-method-settings.test.tsx) are included.
  • Backend Verification: Ensure you have unit/integration tests confirming that:
    1. Passing an invalid npub or hex key returns the new invalid_nostr_recipient error code.
    2. Attempting to add a Nostr contact in cloud mode triggers the rejection response from reject_nostr_in_cloud_mode.
    3. The 035 and 036 migrations apply correctly over an established dataset.

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

Findings

No PR-specific issues found in the changed lines I reviewed.

Test Coverage

The PR adds focused backend tests for migration safety, duplicate Nostr targets, and cloud-mode rejection, plus frontend tests for Nostr settings and provider visibility. That coverage looks appropriate for the changed behavior.

Verification

I attempted to run checks, but the environment blocked them:

  • cargo check failed because crates.io could not be resolved.
  • Frontend Jest tests failed because jest is not installed in frontend/node_modules.

So this review is based on static diff inspection only.

@schjonhaug schjonhaug merged commit ff7e8da into master Jun 16, 2026
6 checks passed
@schjonhaug schjonhaug deleted the schjonhaug/issue-417 branch June 16, 2026 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add self-hosted Nostr DM notification provider

1 participant