Expose WebLN interface via React Context #749
Conversation
components/webln/lnbits.js
Outdated
| this._adminKey = config.adminKey | ||
| await this._updateEnabled() | ||
| // XXX This is insecure, XSS vulns could lead to loss of funds! | ||
| // -> check how mutiny encrypts their wallet and/or check if we can leverage web workers |
There was a problem hiding this comment.
Mutiny just stores secrets in IndexedDB ... but they have a much smaller XSS attack surface.
If we are storing secrets on the client, the best we can do afaict:
- have user encrypt it on client for storing in browser
- on visit, prompt them to decrypt it, loading it into a web worker where spending conditions are enforced
If this is the best we can do ... we might as well store the encrypted secrets and spending conditions on the server for syncing across multiple devices ... then we also don't have to worry about the browser trashing our storage.
There was a problem hiding this comment.
Mutiny just stores secrets in IndexedDB
There is a password prompt to decrypt the wallet though so there is some encryption going on:
but not sure how much encryption would help against XSS anyway. the wallet/credentials would probably be unlocked/decrypted 99% of the time while the user is on the site. having to unlock the wallet regularly would be pretty annoying - but could be an option. like a 10min timer or something. but then again, not sure if even that would help against XSS since the whole client should probably be considered compromised in that case.
but they have a much smaller XSS attack surface.
Good point
on visit, prompt them to decrypt it, loading it into a web worker where spending conditions are enforced
yeah, or go the web worker path.
If this is the best we can do ... we might as well store the encrypted secrets and spending conditions on the server for syncing across multiple devices ... then we also don't have to worry about the browser trashing our storage.
mhh yes, let's see. I tested NWC with Mutiny a little bit and you basically have to approve every spend from your wallet (or configure budgets). so that seems to be pretty safe to use out of the box.
btw, the NIP mentions that it might make sense to run a own relay:
This NIP does not specify any requirements on the type of relays used. However, if the user is using a custodial service it might make sense to use a relay that is hosted by the custodial service. The relay may then enforce authentication to prevent metadata leaks. Not depending on a 3rd party relay would also improve reliability in this case.
There was a problem hiding this comment.
yeah, or go the web worker path
I think regardless of whether we have a web worker, the secrets will need to be stored somewhere for loading into the web worker in which case we will want to encrypt them. Although you might have something more clever planned already.
We've discussed it a little but anything we allow the app (SN) to do through the web worker, eg payments, will be vulnerable to XSS. We can do some things like making sure the payments are going to SN's node or doing some pre-authorization on the serverside, but afaict we can only add more steps to potential exploits (better than nothing but still).
Regardless of these changes even, we should introduce really strict Content Security Policies to narrow the XSS attack surface. For browsers that don't support CSP we should consider preventing storing/using secrets clientside.
ekzyis
force-pushed
the
533-webln
branch
3 times, most recently
from
f0faf5c
to
2c464f8
Compare
|
Current state of UX: 2024-01-18.05-08-44.mp4
However, what I actually intended was to resemble lightning and thunder. The existing lightning strikes could be used as before. However, for WebLN zaps, they simply mean that the payment was initiated but is still pending. The second lightning strike is then the "thunder" to confirm the payment. Since we probably shouldn't play sound since we're no longer in the MySpace era (but maybe it could be a fun setting?), I simply made the second lightning strike thicker. But I imagined the second lightning strike to additionally outline the previous one. This would then also feel more like a confirmation of what happened previously. This is also similar to how messengers work to indicate the status of messages: One check mark means that the message was sent while two mean that the message was received. If we have two strikes (which completely overlap) then I think this could be quite intuitive for users. |
|
I think your intuition around having two signals is spot on. however, without the explanation, ie just watching the video, I found the double strike confusing. I couldn't tell why the second strike was happening. Another variation:
(2) is something I've been wanting to do for even custodial zaps, giving stackers a chance to cancel zaps. |
ekzyis
force-pushed
the
533-webln
branch
2 times, most recently
from
694d1c2
to
8f04159
Compare
|
Okay, I think this is basically done now from a feature and UX perspective. Only things left to do:
One idea I had was to only store the decryption key on the server which is bound to having a valid session. But I think the single point of failure is always the client in that case. If we automatically decrypt the wallet credentials (for UX reasons), then XSS is still possible. We can't expect users to decrypt their credentials for every zap and then encrypt them again. I believe we can only make XSS harder but never prevent it when we store wallet credentials in the client, as you also mentioned here:
related to #770
I have an appointment now and will think more about this when I am back in a few hours. Will also add showcases for each test case; making sure that everything indeed works as expected. Footnotes
|
ekzyis
force-pushed
the
533-webln
branch
3 times, most recently
from
c9968b9
to
754ae92
Compare
|
Test showcase in multiple parts because I found some bugs and didn't want to re-record everything. Just stopped the capture, fixed the bug and started new capture. |
|
Bug during commenting: when I submit a comment with fees, it says it failed but it actually succeeded. Untitled.mov |
If unattach is only shown if configuration is valid, resetting the configuration is not possible while it's invalid. So we're stuck with a red wallet indicator.
|
Wallet configs are now validated on save: 2024-02-07.21-37-37.mp4However, I realized that using the I will add some static checks though (correct amount of characters etc). |
Oh, I noticed I only tested with zaps and forgot to test other things that require payment since I added the toasts. Will do that now. The toasts also shouldn't use |
It depended on a Apollo cache update function being available. But that is not the case for every WebLN payment.
|
Will do. (Apologies for the stray commit. I didn't check before pushing and now I'd have to force push.) Other changes are:
|
|
Confirmed. Works really well now! I'll do a pass over the code. I think the only thing we might want to add before shipping is allowing selection of one send-only wallet or the other as default, so that people can add two.
LNbits has this notion of wallets which are like budgeted accounts so it's not much worse than NWC in that respect. So, I think we can ship with it. In either case, it might worth adding a warning to either method like
|
There was a problem hiding this comment.
Fantastic work! The UX is unreal. Stackers are going to love this!
I left one note, but the rest is solid. I think the only other things we might want:
- optional nit: a checkbox to say one send-only provider should be preferred over the other
- a warning on these send only methods cautioning folks to make sure they have limited access to funds.
We don't need that as we default to NWC, so I'll leave that up to you.
| setEnabled(undefined) | ||
| }, []) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Do we need the useEffect here? Just double checking because you managed to remove the similar logic in the lnbits provider.
There was a problem hiding this comment.
I could also add logic to loadConfig and saveConfig to open/close relays on NWC url changes. But I think this useEffect here is actually pretty nice since it handles closing the relay automatically with the cleanup function and the dependency also automatically handles not closing the relay if the relay URL did not change on a save.
Looking at loadConfig and saveConfig again, I think they could be refactored even more (since they still have some shared code) and handling the relay connection there would make it more complicated.
So I think this useEffect is a net benefit.
Previously, I tended to (ab)use useEffect more as a substitute for reactive programming via the dependency array. But yes, it's inherently a side effect which is hard to track while reading the code. So I also like how useEffect is used less in the WebLN providers now.
Mhh, makes sense! Since we can not only pay invoices but also fetch their balance with their LNbits admin key, we could additionally enhance this warning if we detect they have 100k+ sats in their wallet (for example). This would also make it clear that SN can see their balance when they use LNbits. They have to trust us that there is no code1 running that stores that somewhere.
Shouldn't be too much work, so I'll include this here 👍 edit: Just reread what you wrote. I won't include fallbacks in this PR though. Only the ability to select which payment method should be used if both (LNbits and NWC) are given. I'll write the code with fallbacks in mind. Footnotes
|
Naming of config object was inconsistent with saveConfig function which was annoying. Also fixed other inconsistencies between LNbits and NWC provider.
The list 'paymentMethods' is not used yet but is already implemented for future iterations.
|
4b95add adds a
But seems to work fine now: 2024-02-08.17-53-58.mp4Some issues I have with the current implementation:
I'll think about this more while I add the warning and eat something. |
|
Added disclaimer in 7a55913:
|
|
Looks great. Ready for merge? |
|
Currently outside but if merge only means merge and not release, then yes |
Supersedes #715 Closes #533 Closes #490 Closes #75
Based on #753, #752, #777, #778
TODO:
NaNin cache update[UX] use second lightning strike as "thunder" or "confirmation" (similar to double check marks in messengers) for WebLN zaps by overlapping them[UX] adapt first lightning strike based on if payment is pending (WebLN) or done (custodial) - for example, make pending zaps slower, last longer, different color etc.[UX] investigate if it makes sense to not immediately generate invoices for async providers (NWC); essentially queueing or batching zaps. For example, we could show a notification on their wallet that when clicked shows that zaps are queued. Only when they are ready to pay, we will initiate a payment which they can approve in their wallet then. If zaps are queued, we could also batch them together and create a single invoice for all of them (needs some updates in the backend to allow hash reuse for acts).not planned, too much for MVP[feature] switch between providersnot needed yet if we only launch with NWC