Terminate conn on invalid response id & don't send a response for it (#276)

Changes:

- Terminate bidi conn on invalid response `id`.
- Avoid sending an "invalid request" response on invalid response `id`.
It should only be done on ambiguous messages before terminating.

Author: nitely

json_rpc/client.nim

@@ -133,23 +133,24 @@ proc callOnProcessMessage*(
 
 const defaultRouter = default(RpcRouter)
 
-proc singleResponseId(resp: ResponseRx2): int {.raises: [JsonRpcError].} =
+proc singleResponseId(resp: ResponseRx2): int {.raises: [InvalidResponse].} =
+  # Note this client only ever sends number IDs, and so it expects number IDs back
   case resp.id.kind
   of riNumber:
     resp.id.num
   of riString:
-    int.low
+    raise (ref InvalidResponse)(msg: "Expected response with int `id`, but got `id` = \"" & resp.id.str & "\"")
   of riNull:
     case resp.kind
     of rkResult:
-      int.low
+      raise (ref InvalidResponse)(msg: "Expected response with int `id`, but got `id` = null")
     of rkError:
       # likely an invalid request error
-      raise (ref JsonRpcError)(msg: JrpcSys.encode(resp.error))
+      raise (ref InvalidResponse)(msg: JrpcSys.encode(resp.error))
 
 proc processMessageResponse(
     client: RpcConnection, batch: ResponseBatchRx
-) {.raises: [JsonRpcError].} =
+) {.raises: [InvalidResponse].} =
   let id = case batch.kind
   of rbkMany:
     var curr = int.low

json_rpc/clients/socketclient.nim

@@ -238,6 +238,8 @@ proc processMessages(client: RpcSocketClient) {.async: (raises: []).} =
 
       let resp = try:
         await client.processMessage(data)
+      except InvalidResponse as exc:
+        raise exc
       except JsonRpcError as exc:
         try:
           await client.framing.sendMsg(client.transport, wrapError(router.INVALID_REQUEST, exc.msg))

json_rpc/clients/websocketclient.nim

@@ -109,6 +109,8 @@ proc processMessages(client: RpcWebSocketClient) {.async: (raises: []).} =
 
       let resp = try:
         await client.processMessage(data)
+      except InvalidResponse as exc:
+        raise exc
       except JsonRpcError as exc:
         try:
           await client.transport.send(wrapError(router.INVALID_REQUEST, exc.msg), Opcode.Binary)

tests/test_bidirectional.nim

@@ -7,6 +7,8 @@
 # This file may not be copied, modified, or distributed except according to
 # those terms.
 
+{.push raises: [], gcsafe.}
+
 import
   chronos/unittest2/asynctests,
   stew/byteutils,
@@ -26,6 +28,25 @@ createRpcSigsFromNim(RpcClient, JrpcFlavor):
   proc rets(s: string): string
   proc invalid(s: int): string
 
+template checkInvalidRequest(client: untyped, req, expectedErr: string): untyped =
+  # check sending `req` terminates the connection with `expectedErr` error
+  var disconnFut = newFuture[void]()
+  client.onDisconnect = proc () {.gcsafe, raises: [].} =
+    disconnFut.complete()
+  let fut1 = client.send(req.toBytes)
+  let fut2 = client.rets("foobar")
+  waitFor fut1
+  waitFor disconnFut
+  try:
+    discard waitFor fut2
+    doAssert false
+  except RpcTransportError as err:
+    doAssert err.parent != nil
+    check err.parent.msg == expectedErr
+  # following requests won't work
+  expect RpcTransportError:
+    discard waitFor client.rets("foobar")
+
 template allTests(client: untyped) =
   test "Successful RPC call":
     let r1 = waitFor client.rets("foobar")
@@ -60,109 +81,80 @@ template allTests(client: untyped) =
       res.get()[1].error.isNone
       res.get()[2].error.isSome
 
-  test "Request with null id terminates the connection":
-    # the response will contain a null id
-    var disconnFut = newFuture[void]()
-    client.onDisconnect = proc () {.gcsafe, raises: [].} =
-      disconnFut.complete()
-    let fut1 = client.send("""{"jsonrpc": "2.0", "method": "foobar", "id": null}""".toBytes)
-    let fut2 = client.rets("foobar")
-    waitFor fut1
-    waitFor disconnFut
-    try:
-      discard waitFor fut2
-      doAssert false
-    except RpcTransportError as err:
-      # check it fails with method not found; id=null response
-      check err.parent.msg == """{"code":-32601,"message":"'foobar' is not a registered RPC method"}"""
-    # following requests won't work
-    expect RpcTransportError:
-      discard waitFor client.rets("foobar")
-
-  test "Sending an ambiguous message terminates the connection":
-    var disconnFut = newFuture[void]()
-    client.onDisconnect = proc () {.gcsafe, raises: [].} =
-      disconnFut.complete()
-    let fut1 = client.send("""{"foo": "boo"}""".toBytes)
-    let fut2 = client.rets("foobar")
-    waitFor fut1
-    waitFor disconnFut
-    try:
-      discard waitFor fut2
-      doAssert false
-    except RpcTransportError as err:
-      # check it fails with parse error; id=null response
-      check err.parent.msg == """{"code":-32600,"message":"',' expected"}"""
-    # following requests won't work
-    expect RpcTransportError:
-      discard waitFor client.rets("foobar")
-
-  test "Sending an ambiguous batch message terminates the connection":
-    var disconnFut = newFuture[void]()
-    client.onDisconnect = proc () {.gcsafe, raises: [].} =
-      disconnFut.complete()
-    let fut1 = client.send("""[{"foo": "boo"}]""".toBytes)
-    let fut2 = client.rets("foobar")
-    waitFor fut1
-    waitFor disconnFut
-    try:
-      discard waitFor fut2
-      doAssert false
-    except RpcTransportError as err:
-      # check it fails with parse error; id=null response
-      check err.parent.msg == """{"code":-32600,"message":"',' expected"}"""
-    # following requests won't work
-    expect RpcTransportError:
-      discard waitFor client.rets("foobar")
-
-  test "Sending a response with id null terminates the connection":
-    var disconnFut = newFuture[void]()
-    client.onDisconnect = proc () {.gcsafe, raises: [].} =
-      disconnFut.complete()
-    const resp = """{"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": null}"""
-    waitFor client.send(resp.toBytes)
-    waitFor disconnFut
-
-  test "Sending a batch response with id null terminates the connection":
-    var disconnFut = newFuture[void]()
-    client.onDisconnect = proc () {.gcsafe, raises: [].} =
-      disconnFut.complete()
-    const resp = """[{"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": null}]"""
-    waitFor client.send(resp.toBytes)
-    waitFor disconnFut
-
   test "Sending an unknown id is ignored":
-    const resp = """{"jsonrpc": "2.0", "result": 7, "id": 123123}"""
-    waitFor client.send(resp.toBytes)
+    const req = """{"jsonrpc": "2.0", "method": "foo", "id": 123123}"""
+    waitFor client.send(req.toBytes)
     # following requests still work
     let r1 = waitFor client.rets("foobar")
     check r1 == "ret foobar"
 
-  test "Sending a batch with an unknown id is ignored":
-    const resp = """[{"jsonrpc": "2.0", "result": 7, "id": 123123}]"""
-    waitFor client.send(resp.toBytes)
+  test "Sending an unknown id within a batch is ignored":
+    const req = """[{"jsonrpc": "2.0", "method": "foo", "id": 123123}]"""
+    waitFor client.send(req.toBytes)
     # following requests still work
     let r1 = waitFor client.rets("foobar")
     check r1 == "ret foobar"
 
-  test "Sending a string id is ignored":
-    const resp = """{"jsonrpc": "2.0", "result": 7, "id": "123123"}"""
-    waitFor client.send(resp.toBytes)
+  test "Sending a notification won't terminate the connection":
+    const req = """{"jsonrpc": "2.0", "method": "rets", "params": ["foo"]}"""
+    waitFor client.send(req.toBytes)
     # following requests still work
     let r1 = waitFor client.rets("foobar")
     check r1 == "ret foobar"
 
-  test "Sending a batch with a string id is ignored":
-    const resp = """[{"jsonrpc": "2.0", "result": 7, "id": "123123"}]"""
-    waitFor client.send(resp.toBytes)
+  test "Sending an invalid notification won't terminate the connection":
+    const req = """{"jsonrpc": "2.0", "method": "rets", "params": [123]}"""
+    waitFor client.send(req.toBytes)
     # following requests still work
     let r1 = waitFor client.rets("foobar")
     check r1 == "ret foobar"
 
+  test "Sending an ambiguous message terminates the connection":
+    # check it fails with parse error; id=null response
+    const req = """{"foo": "boo"}"""
+    const expected = """{"code":-32600,"message":"',' expected"}"""
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending an ambiguous batch message terminates the connection":
+    const req = """[{"foo": "boo"}]"""
+    const expected = """{"code":-32600,"message":"',' expected"}"""
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending a null id terminates the connection":
+    # note this terminates the connection when receiving the null id response
+    const req = """{"jsonrpc": "2.0", "method": "foo", "id": null}"""
+    const expected = """{"code":-32601,"message":"'foo' is not a registered RPC method"}"""
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending a null id within a batch terminates the connection":
+    const req = """[{"jsonrpc": "2.0", "method": "foo", "id": null}]"""
+    const expected = """{"code":-32601,"message":"'foo' is not a registered RPC method"}"""
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending a null id terminates the connection; variant":
+    const req = """{"jsonrpc": "2.0", "method": "rets", "params": ["foo"], "id": null}"""
+    const expected = "Expected response with int `id`, but got `id` = null"
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending a null id within a batch terminates the connection; variant":
+    const req = """[{"jsonrpc": "2.0", "method": "rets", "params": ["foo"], "id": null}]"""
+    const expected = "Expected response with int `id`, but got `id` = null"
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending a string id terminates the connection":
+    # note this terminates the connection when receiving the string id response
+    const req = """{"jsonrpc": "2.0", "method": "foo", "id": "123123"}"""
+    const expected = """Expected response with int `id`, but got `id` = "123123""""
+    checkInvalidRequest(client, req, expected)
+
+  test "Sending a string id within a batch terminates the connection":
+    const req = """[{"jsonrpc": "2.0", "method": "foo", "id": "123123"}]"""
+    const expected = """Expected response with int `id`, but got `id` = "123123""""
+    checkInvalidRequest(client, req, expected)
+
 suite "Test bidirectional socket server/client":
   setup:
-    # XXX Framing.lengthHeaderBE32()
-    const framing = Framing.newLine()
+    const framing = Framing.lengthHeaderBE32()
     var srv = newRpcSocketServer(["127.0.0.1:0"], framing = framing)
     var client = newRpcSocketClient(framing = framing)