[bug]: [JMAP] [PushSubscription/set] issues

zvasilev commented 1 year ago

What happened?

Hello Mauro,

It seems that "PushSubscription/set" is not fully implemented. When I call it for the first time:

[[ "PushSubscription/set", {
  "create": {
    "4f29": {
      "deviceClientId": "iOS-device-id",
      "url": "https://mailtemi.eu/api/v1/notify/jmap/?device=iOS-device-id&client=12c6d086",
      "types": null
    }
  }
}, "0" ]]

Stalwart returns:

{"created":{"0696":{"id":"a"}}}

It should be:

"created": {
    "4f29": {
      "id": "a",
      "keys": null,
      "expires": "2018-07-13T02:14:29Z"
    }
}

Two issues here. Stalwart should call the "app push service" via the "url" property. If it doesn't respond or returns an error, an entry in Stalwart should not be created. Instead, report that push notification creation is not possible. This way, the client can communicate with the user and fallback to fetch. It will also be easier for self-hosted users to troubleshoot.

According to my reading of the RFC, Stalwart should communicate with the Push Service, then Apple APNS/Google GCM (assuming this is the correct device-id for the given app). If each service returns okay to the caller, Stalwart will know that the client will have a verificationCode, and then it makes sense to save that entry for subsequent activation.

For the test, I called "PushSubscription/set" with an update containing only the "expires" property. The returned result was: Two issues here. Stalwart should call the "app push service" via the "url" property. If it doesn't respond or returns an error, an entry in Stalwart should not be created. Instead, report that push notification creation is not possible. This way, the client can communicate with the user and fallback to fetch. It will also be easier for self-hosted users to troubleshoot.

According to my reading of the RFC, Stalwart should communicate with the Push Service, then Apple APNS/Google GCM (assuming this is the correct device-id for the given app). If each service returns okay to the caller, Stalwart will know that the client will have a verificationCode, and then it makes sense to save that entry for subsequent activation.

For the test, I called "PushSubscription/set" with an update containing only the "expires" property. The returned result was:

{"updated":{"a":null}}

In my opinion, if "PushSubscription/set" with an update without "verificationCode" hasn't passed, it should return an error?

How can we reproduce the problem?

I can reproduce the problem by doing the following steps: "PushSubscription/set" create "PushSubscription/set" update

Version

v0.3.x

What database are you using?

SQLite

What blob storage are you using?

Local

Where is your directory located?

SQLite

What operating system are you using?

Linux

Relevant log output

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

zvasilev commented 1 year ago

For example, the MS Graph API does not create a subscription if the push server does not return a validationToken. JMAP goes a step further by requiring a validationCode to pass through APSN/GCM, and accordingly, the client activates it on the server. This should protect the JMAP server from DDoS attacks.

mdecimus commented 12 months ago

Thanks for the report @zvasilev . I am currently working on the anti-spam module but I'll take a look at this issue as soon as the new version is released.

zvasilev commented 12 months ago

One more update. I'm not trying to abuse the server, just implementing my error handling unit test :).

In the attached file is the response from Stalwart. You can see there are a lot of subscriptions. push_subscription_get.json.txt

A subscription should be rejected if the client tries to update with the "expires" property in the past. See the first subscription with id:"a", or return the correct "expires" property .
The "deviceClientId" is a unique (app + device) ID. So the next creation with the same "deviceClientId" should be rejected.
Update the verificationCode to null if on the server it is null, which doesn't make sense.

mdecimus commented 11 months ago

For the test, I called "PushSubscription/set" with an update containing only the "expires" property. The returned result was:

This has now been fixed, thanks.

Two issues here. Stalwart should call the "app push service" via the "url" property. If it doesn't respond or returns an error, an entry in Stalwart should not be created. Instead, report that push notification creation is not possible. This way, the client can communicate with the user and fallback to fetch. It will also be easier for self-hosted users to troubleshoot.

Stalwart validates the URL immediately after it is created, if you are not seeing the request please set the debug level to TRACE and check the server logs.

According to my reading of the RFC, Stalwart should communicate with the Push Service, then Apple APNS/Google GCM (assuming this is the correct device-id for the given app). If each service returns okay to the caller, Stalwart will know that the client will have a verificationCode, and then it makes sense to save that entry for subsequent activation.

The example in the RFC is creating the entry and then updating it with the verification code. If you do not create the entry first and obtain its id, it wouldn't be possible to provide the corresponding verificationCode later via an update.

For the test, I called "PushSubscription/set" with an update containing only the "expires" property. In my opinion, if "PushSubscription/set" with an update without "verificationCode" hasn't passed, it should return an error?

Stalwart does not treat this as an error because the remote push server is only contacted once. Updating the expires property without having first provided a valid verificationCode does have any effect on the verification behavior and does not pose any threats.

zvasilev commented 11 months ago

I've updated to version 0.4.2, and now "created" returns the full JSON object: {"created":{"1d44":{"expires":"2023-11-10T12:50:39Z","id":"a","keys":null}}}. However, "updated" still returns {"updated":{"a":null}}. On app startup after get deviceId, and match if there is PushSubscription/get with that deviceId, but no verificationCode. It is supposed to retry to call again "PushSubscription/set". But this will be endless loop?

Although the JMAP RFC doesn't explicitly state it, I think it's a good idea for the HTTP request to return a callback to the request to the push proxy server from "PushSubscription/set". If the HTTP code is 200/201, then save it and return "created." In case of an error timeout or an HTTP error code, return "notCreated." MSGraphs seem to do that.

I'm still investigating why Stalwart cannot connect to my push server.

zvasilev commented 11 months ago

I was able to call my push server from Stalwart by just creating a new user. Is it possible that there is a limit on retries, and after that, "PushSubscription/set" does not make any further attempts? There is no log for such attempt, only from the new user. Destroying and recreating the subscription didn't fix the problem with the first user.

mdecimus commented 11 months ago

However, "updated" still returns {"updated":{"a":null}}.

That is correct, unless the server updates a field that the client did not request to change on the set request, the right response is to return null as no extra properties were modified by the request.

Is it possible that there is a limit on retries, and after that, "PushSubscription/set" does not make any further attempts?

Yes, only one request is made to the end point in order to prevent denial of service attacks as per RFC8620:

   As a push subscription causes the JMAP server to make a number of
   requests to a previously unknown endpoint, it can be used as a vector
   for launching a denial-of-service attack.  To prevent this, when a
   subscription is created, the JMAP server immediately sends a
   PushVerification object to that URL.  The JMAP
   server MUST NOT make any further requests to the URL until the client
   receives the push and updates the subscription with the correct
   verification code.

stalwartlabs / jmap-server