[BUG] window.nostr.signEvent does not validate the input and returns undefined for invalid input

[rolznz]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

Currently if you call signEvent with one of the following, it returns undefined rather than throwing an exception. This leads to confusion.

const signedEvent = await window.nostr.signEvent(undefined); const signedEvent = await window.nostr.signEvent({a: 1});

Screenshots [optional]

No response

Steps To Reproduce

1. call window.nostr.enable()
2. Execute one of the following:

const signedEvent = await window.nostr.signEvent(undefined); const signedEvent = await window.nostr.signEvent({a: 1});

Expected behavior

Alby throws an exception saying the input is not a valid nostr event.

Alby information

Alby 2.0.1

Device information

No response

Additional context

No response

Are you working on this?

None

[Nazi-pikachu]

Hi @rolznz I am new to open source , Can i work upon this issue?

[rolznz]

@Nazi-pikachu sure, go ahead! please tag me when you have a PR ready for review.

[fczuardi]

Is there a real world example of pages or nostr web clients breaking because of that?

[rolznz]

@fczuardi I don't think so. It's more likely to happen when developers are creating a new app and using Alby as their nostr provider. So it's for an improved developer experience.

[mquasar]

@rolznz and @Nazi-pikachu This could be a possible patch to achieve the expected behavior. Any thoughts?

diff --git a/src/extension/background-script/actions/nostr/signEventOrPrompt.ts b/src/extension/background-script/actions/nostr/signEventOrPrompt.ts
index 50556d4e..4bc24fcd 100644
--- a/src/extension/background-script/actions/nostr/signEventOrPrompt.ts
+++ b/src/extension/background-script/actions/nostr/signEventOrPrompt.ts
@@ -14,17 +14,18 @@ const signEventOrPrompt = async (message: MessageSignEvent) => {

   // check event and add an ID and pubkey if not present
   const event = message.args.event;
+  if (!event) return {error: "Invalid event"};
   if (!event.pubkey) event.pubkey = nostr.getPublicKey();
-  if (!event.id) event.id = nostr.getEventHash(event);
-
-  if (!validateEvent(event)) {
-    console.error("Invalid event");
-    return {
-      error: "Invalid event.",
-    };
-  }

   try {
+    if (!event.id) event.id = nostr.getEventHash(event);
+    if (!validateEvent(event)) {
+      console.error("Invalid event");
+      return {
+        error: "Invalid event.",
+      };
+    }
+
     const hasPermission = await hasPermissionFor(
       PermissionMethodNostr["NOSTR_SIGNMESSAGE"],
       message.origin.host

[rolznz]

@mquasar I think validateEvent should check if the event is truthy and has the required properties (created_at, kind, tags, content - see https://github.com/nostr-protocol/nips/blob/master/01.md), and should happen before getting the nostr public key / modifying the event, because it makes no sense to do these things if the event is invalid.

[fczuardi]

The current validateEvent is a simple action that checks just a few attributes, see https://github.com/getAlby/lightning-browser-extension/blob/master/src/extension/background-script/actions/nostr/helpers.ts#L63-L80

~Should we repurpose this issue to refactor the Nostr events validator or create a new one just for that?~

[fczuardi]

I just noticed that the code is borrowed from nostr-tools, we could sync it with the latest version, which looks like: https://github.com/nbd-wtf/nostr-tools/blob/master/event.ts#L95-L113 which is slightly newer than Alby's copy (updated 1 month ago vs 5 months ago)

[fczuardi]

@mquasar I think validateEvent should check if the event is truthy and has the required properties (created_at, kind, tags, content - see https://github.com/nostr-protocol/nips/blob/master/01.md), and should happen before getting the nostr public key / modifying the event, because it makes no sense to do these things if the event is invalid.

the current function already checks for created_at, tags and content, from your list it is only missing kind, if we do the sync mentioned above it should be good enough (nostr-tools have a check for kind).

[rolznz]

@fczuardi thanks for checking. The nostr-tools one also checks the pubkey is set. If so then we need to set the pubkey before validating the event like it is now. If we do that then @mquasar 's initial suggestion might be a good way to do it.