NDK

[riccardobl]

Description

Migrate nostr code to NDK.
I've kept it OOP like the original code.

• nwc sender
• nwc receiver
• crosspost
• zap note

closes #1549

Screenshots

Additional Context

Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?

Checklist

Are your changes backwards compatible? Please answer below:
yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
zap notes are not tested by they should work™

For frontend changes: Tested on mobile? Please answer below:
n/a

Did you introduce any new environment variables? If so, call them out explicitly here:
no

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@nostr-dev-kit/[email protected]	network Transitive: environment, eval, filesystem	+26	4.84 MB	pablof7z

View full report↗︎

[socket-security]

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source	CI
Protestware or potentially unwanted behavior	npm/[email protected]	Note: This package prints a protestware console message on install regarding Ukraine for users with Russian language locale	package-lock.json package.json	⚠︎
Protestware or potentially unwanted behavior	npm/[email protected]	Note: The script attempts to run a local post-install script, which could potentially contain malicious code. The error handling suggests that it is designed to fail silently, which is a common tactic in malicious scripts.	package-lock.json package.json	⚠︎

View full report↗︎

Next steps

What is protestware?

This package is a joke, parody, or includes undocumented or hidden behavior unrelated to its primary function.

Consider that consuming this package may come along with functionality unrelated to its primary purpose.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

• @SocketSecurity ignore npm/[email protected]

[riccardobl]

this is the global singleton, connections to specific relays are handled with RelaySets and their life cycle is left to ndk to handle

[riccardobl]

Logging for nwc is a bit tricky because NDK abstracts away everything and uses npm:debug internally. I've got it to work by using a custom npm:debug logger that proxies to the wallet logger, but i am not sure if the verbosity is too high.

[gitguardian]

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
14860151	Triggered	Generic Private Key	6df7a6a	docker/lndk/tls-key.pem	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secret safely. Learn here the best practices.
3. Revoke and rotate this secret.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 suggestion.

Files not reviewed (2)

• package.json: Language not supported
• lib/nostr.js: Evaluated as low risk

[ekzyis]

You mentioned you tested NWC send but when I try to attach, it fails with pubkey not set. This means that pubkey here is not getting set:

static async fromURI(ndk: NDK, uri: string): Promise<NDKNwc> {
    const u = new URL(uri);

    // validate protocol nostr+walletconnect
    if (u.protocol !== "nostr+walletconnect:") {
        throw new Error("Invalid protocol");
    }

    const nwc = new NDKNwc({
        ndk,
        pubkey: u.host ?? u.pathname,
        relayUrls: u.searchParams.getAll("relay") ?? [""],
        secret: u.searchParams.get("secret") ?? "",
    });

    return nwc;
}

It's related to different URL parsing on server vs client since NWC receive works:

server using node:

$ node
Welcome to Node.js v20.18.0.
Type ".help" for more information.
> new URL("nostr+walletconnect://eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848?relay=wss%3A%2F%2Frelay.primal.net&secret=78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f")
URL {
  href: 'nostr+walletconnect://eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848?relay=wss%3A%2F%2Frelay.primal.net&secret=78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f',
  origin: 'null',
  protocol: 'nostr+walletconnect:',
  username: '',
  password: '',
  host: 'eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848',
  hostname: 'eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848',
  port: '',
  pathname: '',
  search: '?relay=wss%3A%2F%2Frelay.primal.net&secret=78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f',
  searchParams: URLSearchParams {
    'relay' => 'wss://relay.primal.net',
    'secret' => '78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f' },
  hash: ''
}

browser console:

See how u.host is only set to the pubkey in node. In the browser, it's set to the empty string and since NDK uses ??, it will pick the empty string.

But even if NDK would use ||, the pubkey will be wrong because of the //. We got around that different URL parsing behavior by copying how Alby does it. They replace the protocol with http:// in which case it works in both environments:

server using node:

$ node
Welcome to Node.js v20.18.0.
Type ".help" for more information.
> new URL("http://eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848?relay=wss%3A%2F%2Frelay.primal.net&secret=78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f")
URL {
  href: 'http://eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848/?relay=wss%3A%2F%2Frelay.primal.net&secret=78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f',
  origin: 'http://eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848',
  hostname: 'eed35e49c2fdb42819210d56ba4efdd1b3eaee92b271dbb6bfc28bd049526848',
  port: '',
  pathname: '/',
  search: '?relay=wss%3A%2F%2Frelay.primal.net&secret=78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f',
  searchParams: URLSearchParams {
    'relay' => 'wss://relay.primal.net',
    'secret' => '78dd6a6c93f02c0831ff1e74c8ae9a388144f67c10da8276f1b4e475ad9f980f' },
  hash: ''
}

browser console:

See how u.host is now set to the pubkey in both cases.

However, that workaround doesn't work with NDK since it checks for the protocol.

And if I use nostr+walletconnect: instead of nostr+walletconnect://, I run into the same issue that NDK uses ?? instead of ||.

So I am not sure how anyone is able to use NDK's NWC implementation in v2.10.5 of @nostr-dev-kit/ndk in the browser currently. This happens in Brave and Firefox. Maybe they aren't using NDKNwc.fromURI? 🤔

The file where it uses pubkey: u.host ?? u.pathname also hasn't changed in 5 months.

How were you able to attach NWC send?

update: I created a PR to fix this in NDK: nostr-dev-kit/ndk#285

[riccardobl]

Very strange...
It works on firefoxdev and chrome 🤔

2024-11-29.10-17-50.mp4

and brave 173.89

but not on the regular firefox(???)

[riccardobl]

But i remember this being an issue in those browsers too, maybe it is an update they are rolling out or a platform specific issue. I just saw it working and trusted the library to do the right thing.
Luckily NDK exposes the nwc class, so , to avoid surprises, i've reapplied the alby workaround modified for multiple relays, using the nwc constructor instead of the helper method

EDIT: this is very likely related to this
https://docs.google.com/document/u/0/d/1LjxHl32fE4tCKugrK_PIso7mfXQVEeoD1wSnX2y0ZU8/mobilebasic?resourcekey=0-d1gP4X2sG7GPl9mlTeptIA#heading=h.a67ulu2yrl9p
https://groups.google.com/a/chromium.org/g/blink-dev/c/svzicLXbKjw/m/aKt9fw7tBgAJ?pli=1

that shipped in october.

tl;dr: they fixed the URL parser to work the same way for standard and non standard protocols, following the specs (like nodejs is already doing).
It'll take some time for all the browsers to catch up (if they'll do, it seems at least firefox is doing it...), so i think the alby workaround is still the safest option for sn.

Copilot reviewed 6 out of 10 changed files in this pull request and generated no suggestions.

Files not reviewed (4)

• package.json: Language not supported
• components/use-crossposter.js: Evaluated as low risk
• lib/url.js: Evaluated as low risk
• wallets/nwc/client.js: Evaluated as low risk

Comments skipped due to low confidence (1)

wallets/nwc/server.js:29

• The error message concatenation might fail if error.code or error.message are undefined. Consider using default values.

if (error) throw new Error(`${error.code} ${error.message}`)

[ekzyis]

Attaching NWC send+recv works now. I also tested zapping with NWC and since attaching recv works, the server was able to create the test invoice.

However, the worker doesn't seem to be able to connect to the relay. It blocks at

await nwc.blockUntilReady(timeout)

until it times out. Increasing the timeout did not help, even if I increase it to 30 seconds in wallets/server.js:175.

Can you check if that also happens on your side?

Copilot reviewed 6 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (5)

• package.json: Language not supported
• components/use-crossposter.js: Evaluated as low risk
• worker/nostr.js: Evaluated as low risk
• worker/index.js: Evaluated as low risk
• lib/url.js: Evaluated as low risk

Copilot reviewed 6 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (5)

• package.json: Language not supported
• components/use-crossposter.js: Evaluated as low risk
• wallets/nwc/client.js: Evaluated as low risk
• worker/nostr.js: Evaluated as low risk
• worker/index.js: Evaluated as low risk

[ekzyis]

Just one last suggestion (sorry for not mentioning earlier)

[ekzyis]

In case someone is wondering: I am keeping this branch up-to-date with master because I am building my code for #1558 on this.

edit: Mhh, or maybe I don't need to build on this since NDK handles the life cycle of websockets already and I don't think I want to mess with that. So I can just ignore NWC in my PR.

edit 2: Okay, I will remove the withTimeout calls in the NWC code so there will be some conflicts with this PR but they should be really small and easy to resolve.