bug: relay publish fails with 400 Bad Request when message contains meta field

[fbarbu15]

Sending this curl on wakuorg/nwaku:latest:
curl -vv -X POST "http://localhost:43393/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto" -H "accept: text/plain" -H "content-type: application/json" -d '{"payload": "SGVsbG8gV29ybGQh", "contentTopic": "/test/1/waku-relay/proto", "timestamp": 1699891642113786880, "meta": "dGVzdA=="}'
yields 400 Bad Request

On the logs side I can see

DBG 2023-11-13 16:09:39.667+00:00 Serving API request                        tid=1 file=serverprivate.nim:186 peer=172.17.0.1:45944 meth=POST uri=/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto path_params="[(pubsubtopic, /waku/2/default-waku/proto)]" query_params=[] content_body="(application/json, 129 bytes)"
DBG 2023-11-13 16:09:39.667+00:00 JSON field not recognized by the current version of nwaku. Consider upgrading tid=1 file=types.nim:99 fieldName=meta typeName=None
DBG 2023-11-13 16:09:39.667+00:00 Received error response from handler       tid=1 file=serverprivate.nim:307 status=400 meth=POST peer=172.17.0.1:45944 uri=/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto error_status="400 Bad Request" error_content_type=text/plain error_message=

Notes!!

• On gowaku this works fine
• Using RPC instead of REST will return 200 and it will publish the message. However the message reaches peers without the meta field.

[fbarbu15]

This also happens if instead of meta, the payload contains any extra field. Ex
curl -vv -X POST "http://localhost:43393/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto" -H "accept: text/plain" -H "content-type: application/json" -d '{"payload": "SGVsbG8gV29ybGQh", "contentTopic": "/test/1/waku-relay/proto", "timestamp": 1699891642113786880, "extra": "extra"}'

Also in the meantime I tested and got the same results with ephemeral and rate_limit_proof which can be part of the message according to https://rfc.vac.dev/spec/17/#wakumessage

[gabrielmer]

meta and extra fields are not valid as per the API's documentation https://waku-org.github.io/waku-rest-api/#post-/relay/v1/messages/-pubsubTopic-

Discussed the issue with @NagyZoltanPeter

The question is whether it's better to:

1. Return 400, which is the usual behavior when an invalid parameter is sent (so the user doesn't think that it's correct and that we're taking it into account)
2. Ignore the invalid fields to maintain interoperability, and log a warning so the operator can become aware that those parameters are not supported

I'm not sure why support for those fields were not added to the API when designing the endpoints, and if supporting them is something even desired.

Finally, not sure which changes imply defining a new version for the API and which ones can be done without having to define a new version.

CC @jm-clius @alrevuelta

Let me know what you all think :))

[jm-clius]

Most likely this was an oversight, as the meta field is a very late addition to the Waku Message specification. That said, we should definitely support meta in the API as it's an application-defined field (I'd say it's indeed a bug not to).
As for undefined/unsupported fields, I'd prefer simply logging a warning rather than return an error. This will allow easier upgradeability in future.

Finally, not sure which changes imply defining a new version for the API and which ones can be done without having to define a new version.

Generally - if it breaks backwards compatibility (i.e. older clients can't use the API with the new changes), it should be a new API version. These decisions shouldn't be taken lightly, though, and we should avoid breaking changes on the API as much as possible. Adding new fields, for example, is not a breaking change as long as these fields default to something sensible if not populated. In this case, this is a bug/oversight that should be fixed and won't affect the API versioning.

[gabrielmer]

Weekly Update

• achieved: analyzed issue and started implementing fixes
• next: continue implementing the solution

[gabrielmer]

PR was opened. We still need to avoid sending an error when an extra field is passed.
There seems to be an issue with the nim-json-serialization dependency, which throws an exception when an unknown field is passed.

We parse the field's name correctly and run the following line printing a warning that it's an unrecognized field

nwaku/waku/waku_api/rest/relay/types.nim

Line 95 in 9c0e943

unrecognizedFieldWarning()

However, right after that line is run (and when the next iteration of the for starts), an exception is thrown with the following stack trace:

/Users/gabrielmer/status/nwaku/vendor/nim-chronos/chronos/internal/asyncengine.nim(1210) runForever
/Users/gabrielmer/status/nwaku/vendor/nim-chronos/chronos/internal/asyncengine.nim(150) poll
/Users/gabrielmer/status/nwaku/vendor/nim-presto/presto/serverprivate.nim(197) processRestRequest
/Users/gabrielmer/status/nwaku/waku/waku_api/rest/relay/handlers.nim(124) restRelayV1MessagesPubsubTopicPostHandler
/Users/gabrielmer/status/nwaku/waku/waku_api/rest/rest_serdes.nim(47) decodeRequestBody
/Users/gabrielmer/status/nwaku/vendor/nim-serialization/serialization.nim(51) decodeFromJsonBytes
/Users/gabrielmer/status/nwaku/vendor/nim-serialization/serialization.nim(31) readValue
/Users/gabrielmer/status/nwaku/vendor/nim-json-serialization/json_serialization/parser.nim(401) readValue
/Users/gabrielmer/status/nwaku/vendor/nim-json-serialization/json_serialization/reader_desc.nim(126) raiseUnexpectedValue

and with the message:

',' expected

Will discuss it with nim-json-serialization mantainers