bug: light push response is empty when protobuf decode fails

[fryorcraken]

Problem

When js-waku client tries to use light push with a waku message that has no content topic, nwaku sends an empty response back.

Expected: an error in the light push response to point towards decoding issue.

Impact

To reproduce

If you can reproduce the behavior, steps to reproduce:

1. Start nwaku with light push
2. Send a light push rpc request that contains a missing protobuf field such as content topic
3. get an empty rpc light push response

Expected behavior

rpc light push response contains an error

Screenshots/logs

nwaku_Waku_Light_Push_[node_only]_Push_successfully.log

ERR 2023-04-03 20:00:14.214+10:00 failed to decode rpc                       topics="waku lightpush" tid=147839 file=protocol.nim:43

nwaku version/commit hash

v0.16.0

[s-tikhomirov]

Send a light push rpc request

What's the RPC command to do that? I don't see it in the RPC spec.

[Ivansete-status]

Although this issue has the "rpc" word in many places and even within the nwaku code, this issue is not directly replicable throughout the "JSON RPC" service. The issue, on the other hand, is related to the lightpush protocol directly, i.e. the fix candidate will likely come from near the next point

nwaku/waku/waku_lightpush/protocol.nim

Line 42 in 9085b1b

if reqDecodeRes.isErr():

@fryorcraken - How can easily send different js-waku requests to nwaku? We have the next example but I think we cannot enforce sending an empty content topic: https://examples.waku.org/light-js/

[fryorcraken]

Yes it's RPC as PushRPC payload https://rfc.vac.dev/spec/19/

@fryorcraken - How can easily send different js-waku requests to nwaku? We have the next example but I think we cannot enforce sending an empty content topic: examples.waku.org/light-js

I would recommend to use nwaku as a client to reproduce/test.

Here is how we set the content topic to send messages in js-waku: https://github.com/waku-org/js-waku/blob/d049ebbc3417e5c20eccba3aa1b9fc5382e8d7fc/packages/tests/tests/light_push.node.spec.ts#L22

However an empty content topic is now explicitly forbidden: https://github.com/waku-org/js-waku/blob/d049ebbc3417e5c20eccba3aa1b9fc5382e8d7fc/packages/core/src/lib/message/version_0.ts#L79C1-L79C1

[b4s36t4]

nwaku/waku/waku_lightpush/protocol.nim

Lines 42 to 45 in dac072f

	if reqDecodeRes.isErr():
	error "failed to decode rpc"
	waku_lightpush_errors.inc(labelValues = [decodeRpcFailure])
	return

The issue is here if we are failed to parse the proto we're just returning ideally we should return the response

nwaku/waku/waku_lightpush/protocol.nim

Lines 76 to 77 in dac072f

	let rpc = PushRPC(requestId: req.requestId, response: some(response))
	await conn.writeLp(rpc.encode().buffer)

just like we did here.

Extending the response and sending it back should solve the issue.

[s-tikhomirov]

Looking at the existing two tests for Lightpush, I don't quite understand how they work (which may be explained by my lack of Nim knowledge, I admit, but still).

We have two tests: for a successful and for an erring case. What I don't understand is how the outcome of the test depends on what actually happens.

In particular, for the success-case, the handler returns an OK (as it seems to me) irrespective of the actual result of the push:

nwaku/tests/test_waku_lightpush.nim

Line 47 in 57e511f

return ok()

And then we check that the result is indeed OK:

nwaku/tests/test_waku_lightpush.nim

Line 66 in 57e511f

requestRes.isOk()

Likewise, for the fail-case test, the handler returns an error:

nwaku/tests/test_waku_lightpush.nim

Line 91 in 57e511f

return err(error)

And then we check that the result is indeed an error:

nwaku/tests/test_waku_lightpush.nim

Line 110 in 57e511f

requestRes.isErr()

For me it looks like these tests always pass by definition. What am I missing?

[s-tikhomirov]

Also, I have a meta-question here. Shall we distinguish between "no content topic" and "empty content topic"? For me it seems that the former is a decoding issue, while the latter is more high-level one, related to interpretation of data.

Should we treat an empty content topic as a decoding error too? After all, the protobuf in this case is decoded successfully, it just so happens that one of the fields equals some special value (an empty string), which the spec forbids.

If we treat an empty content topic similarly to a missing one, then I would introduce a new check here, so that we additionally check whether topic == "":

nwaku/waku/waku_core/message/codec.nim

Lines 46 to 49 in 11e7394

	if not ?pb.getField(2, topic):
	return err(ProtobufError.missingRequiredField("content_topic"))
	else:
	msg.contentTopic = topic

Otherwise, we could check for topic == "" in the Lightpush protocol, inserting an additional check between these lines:

nwaku/waku/waku_lightpush/protocol.nim

Lines 56 to 57 in 11e7394

	message = req.request.get().message
	debug "push request", peerId=conn.peerId, requestId=req.requestId, pubsubTopic=pubsubTopic

Any thoughts?

[SionoiS]

Looking at the existing two tests for Lightpush, I don't quite understand how they work (which may be explained by my lack of Nim knowledge, I admit, but still).

I've stumble upon tests like that before. They don't test "end to end", in this case the best would be to actually check if the message was send via RELAY by having the node subscribe to the topic then check the cache.

Also, I have a meta-question here. Shall we distinguish between "no content topic" and "empty content topic"? For me it seems that the former is a decoding issue, while the latter is more high-level one, related to interpretation of data.

IMO a clear separation between protocol error and in this case decode error would be best.

[s-tikhomirov]

in this case the best would be to actually check if the message was send

Ok, I'm gonna go step by step here.

In a test, how do I create a server that uses the actual handler described in the protocol, instead of a mock handler that always returns Ok or Err?

In particular, how do I bring the logic from here:

nwaku/waku/waku_lightpush/protocol.nim

Lines 38 to 77 in 11e7394

	proc initProtocolHandler*(wl: WakuLightPush) =
	proc handle(conn: Connection, proto: string) {.async, gcsafe, closure.} =
	let buffer = await conn.readLp(MaxRpcSize.int)
	let reqDecodeRes = PushRPC.decode(buffer)
	if reqDecodeRes.isErr():
	error "failed to decode rpc"
	waku_lightpush_errors.inc(labelValues = [decodeRpcFailure])
	return
	let req = reqDecodeRes.get()
	if req.request.isNone():
	error "invalid lightpush rpc received", error=emptyRequestBodyFailure
	waku_lightpush_errors.inc(labelValues = [emptyRequestBodyFailure])
	return
	waku_lightpush_messages.inc(labelValues = ["PushRequest"])
	let
	pubSubTopic = req.request.get().pubSubTopic
	message = req.request.get().message
	debug "push request", peerId=conn.peerId, requestId=req.requestId, pubsubTopic=pubsubTopic
	var response: PushResponse
	var handleRes: WakuLightPushResult[void]
	try:
	handleRes = await wl.pushHandler(conn.peerId, pubsubTopic, message)
	except Exception:
	response = PushResponse(is_success: false, info: some(getCurrentExceptionMsg()))
	waku_lightpush_errors.inc(labelValues = [messagePushFailure])
	error "pushed message handling failed", error= getCurrentExceptionMsg()
	if handleRes.isOk():
	response = PushResponse(is_success: true, info: some("OK"))
	else:
	response = PushResponse(is_success: false, info: some(handleRes.error))
	waku_lightpush_errors.inc(labelValues = [messagePushFailure])
	error "pushed message handling failed", error=handleRes.error
	let rpc = PushRPC(requestId: req.requestId, response: some(response))
	await conn.writeLp(rpc.encode().buffer)

to here:

nwaku/tests/test_waku_lightpush.nim

Line 94 in 57e511f

server = await newTestWakuLightpushNode(serverSwitch, handler)

?

[SionoiS]

You don't have to. The handler you pass to new is wrapped by the one in initProtocolHandler L62

Currently the test is very narrow, what I meant to say was that it maybe better broaden the scope.

Narrow:

1. light push client -> light push server
2. send whatever via light push
3. if handler called ✔️

Broader:

1. light push client -> light push server
2. client send specific message on topic
3. if handler called with the same topic and message ✔️

Very Broad:

1. light push client node connect to server node with relay & light push
2. server node subscribe to topic.
3. client node send message on topic via light push
4. get message on server node via relay api
5. if message match ✔️

Hope it helps :)

[s-tikhomirov]

The handler you pass to new is wrapped by the one in initProtocolHandler L62

I'm not sure I understand how "wrapping" works exactly...

In the test, we initialize a server passing handler as an argument, where handler is defined just above:

nwaku/tests/test_waku_lightpush.nim

Lines 45 to 50 in 57e511f

	let handler = proc(peer: PeerId, pubsubTopic: PubsubTopic, message: WakuMessage): Future[WakuLightPushResult[void]] {.async.} =
	handlerFuture.complete((pubsubTopic, message))
	return ok()
	let
	server = await newTestWakuLightpushNode(serverSwitch, handler)

How does this linked to initProtocolHandler

nwaku/waku/waku_lightpush/protocol.nim

Line 38 in a01b63a

proc initProtocolHandler*(wl: WakuLightPush) =

and L62 in particular?

nwaku/waku/waku_lightpush/protocol.nim

Line 62 in a01b63a

handleRes = await wl.pushHandler(conn.peerId, pubsubTopic, message)

[SionoiS]

The handler you pass to new is wrapped by the one in initProtocolHandler L62

I'm not sure I understand how "wrapping" works exactly...

When WakuLightPush is created handler A is saved in the object.

nwaku/waku/waku_lightpush/protocol.nim

Line 36 in a01b63a

pushHandler*: PushMessageHandler

Then initProtocolHandler is called and creates handler B that calls A and takes its place in the object.

nwaku/waku/waku_lightpush/protocol.nim

Line 79 in a01b63a

wl.handler = handle

Resulting in handler B "wrapping" handler A

Maybe this GC magic is unfamiliar to you? I know I was a bit loss at first because of my Rust (no GC) background.

[fryorcraken]

Related: #2059

[s-tikhomirov]

Maybe this GC magic is unfamiliar to you?

That's quite likely!

This what I think I understand (please correct me if I'm wrong). In protocol.nim we define the WakuLightPush protocol. Its initialization is defined in proc new:

nwaku/waku/waku_lightpush/protocol.nim

Lines 86 to 87 in a01b63a

	let wl = WakuLightPush(rng: rng, peerManager: peerManager, pushHandler: pushHandler)
	wl.initProtocolHandler()

Here, the pushHandler is the "core" handler, so to say (or handler A). AFAICT, it is responsible for actually pushing a message to the network. Meanwhile, inside initProtocolHandler, we define proc handle that preliminary checks that the request is correctly formatted. If all checks pass, pushHandler is called from within handle:

nwaku/waku/waku_lightpush/protocol.nim

Line 62 in a01b63a

handleRes = await wl.pushHandler(conn.peerId, pubsubTopic, message)

Finally, we assign handle (handler B) as handler:

nwaku/waku/waku_lightpush/protocol.nim

Line 79 in a01b63a

wl.handler = handle

Here, I can see how handler wraps pushHandler.

But how is this all linked to what happens in the test? While I was writing this question, I think I understood it :D I'll outline it here anyway.

In the test, we initialize the server here:

nwaku/tests/test_waku_lightpush.nim

Line 50 in 57e511f

server = await newTestWakuLightpushNode(serverSwitch, handler)

where inside newTestWakuLightpushNode we have:

nwaku/tests/test_waku_lightpush.nim

Line 20 in 57e511f

proto = WakuLightPush.new(peerManager, rng, handler)

And the handler is defined here:

nwaku/tests/test_waku_lightpush.nim

Lines 45 to 47 in 57e511f

	let handler = proc(peer: PeerId, pubsubTopic: PubsubTopic, message: WakuMessage): Future[WakuLightPushResult[void]] {.async.} =
	handlerFuture.complete((pubsubTopic, message))
	return ok()

My confusion was caused by the fact that what is called handler in the test, is called pushHandler in the protocol definition. I failed to see that handler in the test plays the role of the "internal" handler, which for testing purposes simply returns Ok or Err. But that's not a problem for testing the "outer" handler functionality (checking decoding errors, etc), as it is "wrapped" around handler in any case.

Thank you so much @SionoiS for your time, your comments are truly helpful!

[s-tikhomirov]

A more high-level question: what is the expected behavior w.r.t. client-server interaction in case the client sends an invalid request?

As an example, consider the case when the request can't be decoded.

From what I see, on the server side, the handler will return and will not generate any PushResponse:

nwaku/waku/waku_lightpush/protocol.nim

Lines 42 to 45 in a01b63a

	if reqDecodeRes.isErr():
	error "failed to decode rpc"
	waku_lightpush_errors.inc(labelValues = [decodeRpcFailure])
	return

On the client side, however, it seems that the client is expecting a push response for erring cases (or is it?):

nwaku/waku/waku_lightpush/client.nim

Lines 60 to 65 in a01b63a

	let response = pushResponseRes.response.get()
	if not response.isSuccess:
	if response.info.isSome():
	return err(response.info.get())
	else:
	return err("unknown failure")

How should such cases be handled generally? I mean the scenarios when a client sends a request to a server, but the request is invalid in some way, therefore no message is pushed to the Relay network. How should the server respond? Should this case be handled differently from a scenario when a server did push a message, but the push failed due to issues on the Relay side?

[SionoiS]

My confusion was caused by the fact that what is called handler in the test, is called pushHandler in the protocol definition. I failed to see that handler in the test plays the role of the "internal" handler, which for testing purposes simply returns Ok or Err. But that's not a problem for testing the "outer" handler functionality (checking decoding errors, etc), as it is "wrapped" around handler in any case.

Thank you so much @SionoiS for your time, your comments are truly helpful!

Actually i just realized that I did not understand how it worked either.

Because

nwaku/waku/waku_lightpush/protocol.nim

Line 33 in 1c4533a

WakuLightPush* = ref object of LPProtocol

and

nwaku/waku/waku_lightpush/protocol.nim

Line 79 in 1c4533a

wl.handler = handle

The handlers call order is;

1. libp2p calls

   nwaku/waku/waku_lightpush/protocol.nim

   Line 39 in 1c4533a

   proc handle(conn: Connection, proto: string) {.async, gcsafe, closure.} =

2. nwaku/waku/waku_lightpush/protocol.nim

   Line 62 in 1c4533a

   handleRes = await wl.pushHandler(conn.peerId, pubsubTopic, message)

3. nwaku/waku/node/waku_node.nim

   Lines 828 to 839 in 1c4533a

   	var pushHandler: PushMessageHandler
   	if node.wakuRelay.isNil():
   	debug "mounting lightpush without relay (nil)"
   	pushHandler = proc(peer: PeerId, pubsubTopic: string, message: WakuMessage): Future[WakuLightPushResult[void]] {.async.} =
   	return err("no waku relay found")
   	else:
   	pushHandler = proc(peer: PeerId, pubsubTopic: string, message: WakuMessage): Future[WakuLightPushResult[void]] {.async.} =
   	discard await node.wakuRelay.publish(pubsubTopic, message.encode().buffer)
   	return ok()
   	debug "mounting lightpush with relay"
   	node.wakuLightPush = WakuLightPush.new(node.peerManager, node.rng, pushHandler)

How should such cases be handled generally? I mean the scenarios when a client sends a request to a server, but the request is invalid in some way, therefore no message is pushed to the Relay network. How should the server respond? Should this case be handled differently from a scenario when a server did push a message, but the push failed due to issues on the Relay side?

IMO always sending a response is best. We don't do that here for some reason.... Good error messages are also important.
In this case having more standard errors (maybe added to the spec.) would improve things greatly.

[NagyZoltanPeter]

@s-tikhomirov from now on, you can use Rest API to test/debug lightpush.
Also test_rest_lightpush.nim does end to end test and failure cases. You will find it on master.