feat(nwc): validate wallet connect URI pubkey, relays, and secret

[DSanich]

Summary

Improves validation when parsing Nostr Wallet Connect (nostr+walletconnect / legacy nostrwalletconnect) connection strings in NWCClient.parseWalletConnectUrl, closing the gaps described in #539.

Changes

• Wallet pubkey (url.host): must be either a 64-character hex string or a valid npub (decoded via nip19).
• Relay URLs: require at least one non-empty relay after trimming; each value must parse as a URL with an allowed scheme (wss, ws, https, http). Fixes the previous check that never fired for an empty getAll("relay") result.
• Secret: by default the URI must include a valid secret — either 64 hex chars or nsec; nsec is normalized to hex so the rest of the client (e.g. hexToBytes / signing) stays consistent.
• ParseWalletConnectUrlOptions: optional requireSecret (default true). Exposed on NewNWCClientOptions as parseWalletConnectUrlOptions when constructing from nostrWalletConnectUrl.

Closes #539

Summary by CodeRabbit

• New Features
  • URL parsing now accepts hex and bech32 wallet keys and secrets, normalizes them, and enforces WS/WSS relays.
  • Added an option to relax or require presence of a secret when parsing; URL generation now errors if no secret is available.
• Bug Fixes
  • Stricter validation for relay entries and secret/pubkey formats to prevent malformed inputs.
• Tests
  • Expanded tests covering valid/invalid URLs, bech32 decoding rules, relay edge cases, and constructor behaviors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 548a9722-04e4-4827-b293-2e6e36193dea

📥 Commits

Reviewing files that changed from the base of the PR and between affde56 and db8d3da.

📒 Files selected for processing (2)

• src/nwc/NWCClient.test.ts
• src/nwc/NWCClient.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/nwc/NWCClient.test.ts

---

📝 Walkthrough

Walkthrough

Added robust NWC URI parsing and normalization: relay URL trimming/validation, wallet pubkey normalization (hex/bech32 rejection cases tested), secret normalization (hex and nsec), and an optional parse option to require or allow missing secrets; constructor and URL builder updated to enforce secret presence where applicable.

Changes

Cohort / File(s)	Summary
Tests — NWC parsing cases src/nwc/NWCClient.test.ts	Replaced hardcoded WalletConnect URI with a generated nwcTestUri builder; expanded tests to assert generated hex walletPubkey/secret, added NIP-47 decoding rejection cases (npub/nsec misuse), tests for optional requireSecret:false, and many negative tests for relay, pubkey, and secret validation. Added test asserting getNostrWalletConnectUrl() throws without a client secret.
Core parsing & client src/nwc/NWCClient.ts	Added exported ParseWalletConnectUrlOptions and parseWalletConnectUrl(..., parseOptions). Introduced helper normalization/validation for relay URLs, wallet pubkey, and secret (nsec→hex); updated constructor/new options to accept parseWalletConnectUrlOptions, merged parsed results, and enforced secret presence for URL building and when requireSecret constraints apply. Secret decoding uses bytesToHex(...).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

• rolznz

Poem

🐰 I hopped through URIs, tidy and spry,
Relays trimmed neat, secrets turned to hex,
Pubkeys checked under moonlit sky,
Tests sing errors when formats perplex,
A rabbit cheers — parsing's fixed! 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Changes are primarily focused on validation logic for wallet connect URIs. However, the review feedback suggests the implementation goes beyond NIP-47 spec requirements by allowing HTTP relay schemes and parsing npub-encoded pubkeys, which may exceed the scope of issue #539's core requirements.	Review and align relay scheme validation with NIP-47 spec (likely wss/ws only) and reconsider npub pubkey parsing support if not explicitly required by the linked issue.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: validation of wallet connect URI components (pubkey, relays, and secret).
Linked Issues check	✅ Passed	The PR addresses all three requirements from issue #539: pubkey format/length validation, configurable secret validation with proper format checks, and relay URL validation with at least one non-empty valid relay.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/nwc/NWCClient.test.ts`:
- Around line 70-166: Tests embed a real-looking hardcoded wallet-connect secret
repeatedly; replace those literals with a synthetic/generated fixture and build
URLs programmatically. Create a shared test fixture (e.g., generateSecretKey() +
bytesToHex(...) or nip19.nsecEncode(...) used in the first test) and reuse it
when constructing URLs for the tests that call NWCClient.parseWalletConnectUrl
and NWCClient constructor; update all occurrences that currently hardcode the
secret string so they interpolate the fixture instead, ensuring tests still
cover hex and nsec normalization, missing/invalid secret, and
parseWalletConnectUrl options.

In `@src/nwc/NWCClient.ts`:
- Around line 156-159: The new parse-only constructor path allows creating an
NWCClient with secret === undefined (via
parseWalletConnectUrlOptions.requireSecret = false), but NWCClient methods like
getNostrWalletConnectUrl() and other secret-dependent APIs are not handling that
and will serialize "&secret=undefined" or crash; update the NWCClient
constructor and relevant methods (constructor, getNostrWalletConnectUrl, and any
secret-dependent operations referenced around the parse logic) to enforce one of
two behaviors: either reject/throw if an instance is constructed without a
secret (when methods that need it are called) or clearly mark/guard the instance
as "secretless" and make getNostrWalletConnectUrl() and other serializers omit
the secret (not serialize undefined) and throw early with a clear error when
secret-required operations are invoked; implement the chosen approach
consistently for parseWalletConnectUrlOptions and requireSecret handling so no
instance can accidentally serialize "&secret=undefined" or perform secret-only
ops without an explicit runtime check.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 17bbfa96-3fb5-47ae-ae06-60cfdfbbd665

📥 Commits

Reviewing files that changed from the base of the PR and between da2352b and affde56.

📒 Files selected for processing (2)

• src/nwc/NWCClient.test.ts
• src/nwc/NWCClient.ts

[rolznz]

Thanks for the PR!

There is a lot of excessive code here I think - it should follow the NIP-47 spec and not do extra stuff like npub parsing or relay URLs starting with http.

[DSanich]

@rolznz Thanks for review! Working on it

[DSanich]

@rolznz Could you please review it again? 🙏 I made some edits in this commit