From 5dd645cfc3f95f0cf037574a9968178c5dd6ed04 Mon Sep 17 00:00:00 2001
From: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Date: Wed, 1 May 2024 23:05:22 +0530
Subject: [PATCH] fix(waku_keystore): sigsegv on different appInfo (#2654)

* fix(waku_keystore): sigsegv on different appInfo

* fix: field specific errors

* fix: more verbose error logs
---
 tests/test_waku_keystore.nim          | 57 +++++++++++++++++++++++++++
 waku/waku_keystore/keystore.nim       | 51 ++++++++++++++++++++----
 waku/waku_keystore/protocol_types.nim |  1 +
 3 files changed, 101 insertions(+), 8 deletions(-)

diff --git a/tests/test_waku_keystore.nim b/tests/test_waku_keystore.nim
index ceda34871..36e51b9ed 100644
--- a/tests/test_waku_keystore.nim
+++ b/tests/test_waku_keystore.nim
@@ -246,3 +246,60 @@ procSuite "Credentials test suite":
     check:
       recoveredCredentialsRes.isErr()
       recoveredCredentialsRes.error.kind == KeystoreCredentialNotFoundError
+
+  test "if a keystore exists, but the keystoreQuery doesn't match it":
+    let filepath = "./testAppKeystore.txt"
+    defer:
+      removeFile(filepath)
+
+    # We generate random identity credentials (inter-value constrains are not enforced, otherwise we need to load e.g. zerokit RLN keygen)
+    let
+      idTrapdoor = randomSeqByte(rng[], 32)
+      idNullifier = randomSeqByte(rng[], 32)
+      idSecretHash = randomSeqByte(rng[], 32)
+      idCommitment = randomSeqByte(rng[], 32)
+      idCredential = IdentityCredential(
+        idTrapdoor: idTrapdoor,
+        idNullifier: idNullifier,
+        idSecretHash: idSecretHash,
+        idCommitment: idCommitment,
+      )
+
+    # We generate two distinct membership groups
+    let contract = MembershipContract(
+      chainId: "5", address: "0x0123456789012345678901234567890123456789"
+    )
+    let index = MembershipIndex(1)
+    var membershipCredential = KeystoreMembership(
+      membershipContract: contract, treeIndex: index, identityCredential: idCredential
+    )
+
+    let password = "%m0um0ucoW%"
+
+    let keystoreRes = addMembershipCredentials(
+      path = filepath,
+      membership = membershipCredential,
+      password = password,
+      appInfo = testAppInfo,
+    )
+
+    assert(keystoreRes.isOk(), $keystoreRes.error)
+
+    let badTestAppInfo =
+      AppInfo(application: "_bad_test_", appIdentifier: "1234", version: "0.1")
+
+    # We test retrieval of credentials.
+    let membershipQuery = KeystoreMembership(membershipContract: contract)
+
+    let recoveredCredentialsRes = getMembershipCredentials(
+      path = filepath,
+      password = password,
+      query = membershipQuery,
+      appInfo = badTestAppInfo,
+    )
+
+    check:
+      recoveredCredentialsRes.isErr()
+      recoveredCredentialsRes.error.kind == KeystoreJsonValueMismatchError
+      recoveredCredentialsRes.error.msg ==
+        "Application does not match. Expected '_bad_test_' but got 'test'"
diff --git a/waku/waku_keystore/keystore.nim b/waku/waku_keystore/keystore.nim
index 7187b2de2..65371c5a7 100644
--- a/waku/waku_keystore/keystore.nim
+++ b/waku/waku_keystore/keystore.nim
@@ -45,7 +45,6 @@ proc loadAppKeystore*(
 ): KeystoreResult[JsonNode] =
   ## Load and decode JSON keystore from pathname
   var data: JsonNode
-  var matchingAppKeystore: JsonNode
 
   # If no keystore exists at path we create a new empty one with passed keystore parameters
   if fileExists(path) == false:
@@ -77,14 +76,45 @@ proc loadAppKeystore*(
 
         # We check if parsed json contains the relevant keystore credentials fields and if these are set to the passed parameters
         # (note that "if" is lazy, so if one of the .contains() fails, the json fields contents will not be checked and no ResultDefect will be raised due to accessing unavailable fields)
-        if data.hasKeys(["application", "appIdentifier", "credentials", "version"]) and
-            data["application"].getStr() == appInfo.application and
-            data["appIdentifier"].getStr() == appInfo.appIdentifier and
-            data["version"].getStr() == appInfo.version:
+        if not data.hasKeys(["application", "appIdentifier", "credentials", "version"]):
+          return err(
+            AppKeystoreError(
+              kind: KeystoreJsonKeyError, msg: "Missing required fields in keystore"
+            )
+          )
+
+        if data["application"].getStr() != appInfo.application:
+          return err(
+            AppKeystoreError(
+              kind: KeystoreJsonValueMismatchError,
+              msg:
+                "Application does not match. Expected '" & appInfo.application &
+                "' but got '" & data["application"].getStr() & "'",
+            )
+          )
+
+        if data["appIdentifier"].getStr() != appInfo.appIdentifier:
+          return err(
+            AppKeystoreError(
+              kind: KeystoreJsonValueMismatchError,
+              msg:
+                "AppIdentifier does not match. Expected '" & appInfo.appIdentifier &
+                "' but got '" & data["appIdentifier"].getStr() & "'",
+            )
+          )
+
+        if data["version"].getStr() != appInfo.version:
+          return err(
+            AppKeystoreError(
+              kind: KeystoreJsonValueMismatchError,
+              msg:
+                "Version does not match. Expected '" & appInfo.version & "' but got '" &
+                data["version"].getStr() & "'",
+            )
+          )
           # We return the first json keystore that matches the passed app parameters
           # We assume a unique kesytore with such parameters is present in the file
-          matchingAppKeystore = data
-          break
+        return ok(data)
       # TODO: we might continue rather than return for some of these errors
       except JsonParsingError:
         return
@@ -101,7 +131,12 @@ proc loadAppKeystore*(
   except IOError:
     return err(AppKeystoreError(kind: KeystoreIoError, msg: getCurrentExceptionMsg()))
 
-  return ok(matchingAppKeystore)
+  return err(
+    AppKeystoreError(
+      kind: KeystoreKeystoreDoesNotExist,
+      msg: "No keystore found for the passed parameters",
+    )
+  )
 
 # Adds a membership credential to the keystore matching the application, appIdentifier and version filters.
 proc addMembershipCredentials*(
diff --git a/waku/waku_keystore/protocol_types.nim b/waku/waku_keystore/protocol_types.nim
index 51d7faf7c..a3d9fb366 100644
--- a/waku/waku_keystore/protocol_types.nim
+++ b/waku/waku_keystore/protocol_types.nim
@@ -168,6 +168,7 @@ type
     KeystoreOsError = "keystore error: OS specific error"
     KeystoreIoError = "keystore error: IO specific error"
     KeystoreJsonKeyError = "keystore error: fields not present in JSON"
+    KeystoreJsonValueMismatchError = "keystore error: JSON value mismatch"
     KeystoreJsonError = "keystore error: JSON encoder/decoder error"
     KeystoreKeystoreDoesNotExist = "keystore error: file does not exist"
     KeystoreCreateKeystoreError = "Error while creating application keystore"