unhandled TypeError for NotRegistered Device Group Notification Keys

[evolvingfunctions]

node-gcm has been working smoothly for my app server, but I'm seeing an unhandled TypeError anytime the app server tries to send a GCM to a Device Group Notification Key that is no longer registered. I know why the Notification Key does not exist. It is due to multiple users sharing the same mobile device, which is common during testing. Is there any way to have node-gcm respond with the Device Group NotRegistered error without crashing node-gcm?

/app/node_modules/node-gcm/lib/sender.js:39
 if(!response.results) {
                    ^
 TypeError: Cannot read property 'results' of undefined
     at /app/node_modules/node-gcm/lib/sender.js:39:21
     at Request._callback (/app/node_modules/node-gcm/lib/sender.js:165:13)
     at Request.self.callback (/app/node_modules/request/request.js:188:22)
     at emitTwo (events.js:106:13)
     at Request.emit (events.js:191:7)
     at Request.<anonymous> (/app/node_modules/request/request.js:1171:10)
     at emitOne (events.js:96:13)
     at Request.emit (events.js:188:7)
     at IncomingMessage.<anonymous> (/app/node_modules/request/request.js:1091:12)
     at IncomingMessage.g (events.js:291:16)
     at emitNone (events.js:91:20)
     at IncomingMessage.emit (events.js:185:7)
     at endReadableNT (_stream_readable.js:974:12)
     at _combinedTickCallback (internal/process/next_tick.js:80:11)
     at process._tickDomainCallback (internal/process/next_tick.js:128:9)

[eladnava]

Hi @evolvingfunctions, Seems like you are using an outdated version of node-gcm.

Please update to 0.14.4 and let us know if the issue still occurs. The latest version should not be checking response.results if an err is present: https://github.com/ToothlessGear/node-gcm/blob/ff2d7d7a4b8ada75a76a0af57b75a18185cb09ec/lib/sender.js#L36-L45

[evolvingfunctions]

Thanks for replying, @eladnava. I double checked and I'm using version 0.14.4.

I think the problem is that the Google API responds differently for unregistered Notification Keys or Registration Tokens. From a terminal session, I used curl to test the validity of the notification key and the registration token.

Dry run message to reg token that is no longer registered:

curl -v -H "Content-Type: application/json" -H "Authorization: key=<<auth_key_redacted>>" -H -X POST 'https://fcm.googleapis.com/fcm/send' -d '{"dry_run": true, "to": "<<unregistered_reg_token>>" }'
* Rebuilt URL to: POST/
* Could not resolve host: POST
* Closing connection 0
curl: (6) Could not resolve host: POST
*   Trying 216.58.219.10...
* Connected to fcm.googleapis.com (216.58.219.10) port 443 (#1)
* found 173 certificates in /etc/ssl/certs/ca-certificates.crt
* found 704 certificates in /etc/ssl/certs
* ALPN, offering http/1.1
* SSL connection using TLS1.2 / ECDHE_RSA_AES_128_GCM_SHA256
*    server certificate verification OK
*    server certificate status verification SKIPPED
*    common name: *.googleapis.com (matched)
*    server certificate expiration date OK
*    server certificate activation date OK
*    certificate public key: RSA
*    certificate version: #3
*    subject: C=US,ST=California,L=Mountain View,O=Google Inc,CN=*.googleapis.com
*    start date: Wed, 22 Mar 2017 16:53:01 GMT
*    expire date: Wed, 14 Jun 2017 16:17:00 GMT
*    issuer: C=US,O=Google Inc,CN=Google Internet Authority G2
*    compression: NULL
* ALPN, server accepted to use http/1.1
> POST /fcm/send HTTP/1.1
> Host: fcm.googleapis.com
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Type: application/json
> Authorization: key=<<<auth_key_redacted>>>
> Content-Length: 180
> 
* upload completely sent off: 180 out of 180 bytes
< HTTP/1.1 200 OK
< Content-Type: application/json; charset=UTF-8
< Date: Tue, 28 Mar 2017 15:59:03 GMT
< Expires: Tue, 28 Mar 2017 15:59:03 GMT
< Cache-Control: private, max-age=0
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-XSS-Protection: 1; mode=block
< Server: GSE
< Alt-Svc: quic=":443"; ma=2592000; v="37,36,35"
< Accept-Ranges: none
< Vary: Accept-Encoding
< Transfer-Encoding: chunked
< 
{
  "multicast_id":-1,
  "success":0,
  "failure":1,
  "canonical_ids":0,
  "results": [{"error":"NotRegistered"}]
}

Dry run message to device group notification key that is no longer registered:

curl -v -H "Content-Type: application/json" -H "Authorization: key=<<auth_key_redacted>>" -H -X POST 'https://fcm.googleapis.com/fcm/send' -d '{"dry_run": true, "to": "<<unregistered_notification_key>>" }'
* Rebuilt URL to: POST/
* Could not resolve host: POST
* Closing connection 0
curl: (6) Could not resolve host: POST
*   Trying 172.217.4.170...
* Connected to fcm.googleapis.com (172.217.4.170) port 443 (#1)
* found 173 certificates in /etc/ssl/certs/ca-certificates.crt
* found 704 certificates in /etc/ssl/certs
* ALPN, offering http/1.1
* SSL connection using TLS1.2 / ECDHE_RSA_AES_128_GCM_SHA256
*    server certificate verification OK
*    server certificate status verification SKIPPED
*    common name: *.googleapis.com (matched)
*    server certificate expiration date OK
*    server certificate activation date OK
*    certificate public key: RSA
*    certificate version: #3
*    subject: C=US,ST=California,L=Mountain View,O=Google Inc,CN=*.googleapis.com
*    start date: Thu, 16 Mar 2017 08:54:00 GMT
*    expire date: Thu, 08 Jun 2017 08:54:00 GMT
*    issuer: C=US,O=Google Inc,CN=Google Internet Authority G2
*    compression: NULL
* ALPN, server accepted to use http/1.1
> POST /fcm/send HTTP/1.1
> Host: fcm.googleapis.com
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Type: application/json
> Authorization: key=<<<auth_key_redacted>>>
> Content-Length: 190
> 
* upload completely sent off: 190 out of 190 bytes
< HTTP/1.1 200 NotRegistered
< Content-Type: text/plain; charset=ISO-8859-1
< Date: Tue, 28 Mar 2017 16:12:59 GMT
< Expires: Tue, 28 Mar 2017 16:12:59 GMT
< Cache-Control: private, max-age=0
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-XSS-Protection: 1; mode=block
< Content-Length: 0
< Server: GSE
< Alt-Svc: quic=":443"; ma=2592000; v="37,36,35"
< 

Both calls result in HTTP 200, but the notification key response contains an empty body. This produces a case where res.statusCode === 200 and the response.results object does not exist.

[eladnava]

Seems like GCM simply returns 200 NotRegistered without any JSON response for a request to one notification key that is no longer valid: https://firebase.google.com/docs/cloud-messaging/http-server-ref#table9

Currently node-gcm assumes that 200 response code will always contain a JSON response object with a response.results array, which it does not in this case.

I think we should detect this situation and throw an error to the callback in this situation, somewhere along these lines: https://github.com/ToothlessGear/node-gcm/blob/ff2d7d7a4b8ada75a76a0af57b75a18185cb09ec/lib/sender.js#L165-L169

            if (res.statusCode === 200 && res.statusMessage !== 'OK') {
                debug('Invalid status message (' + res.statusCode + ' ' + res.statusMessage + ')');
                return callback(res.statusCode); // maybe we can provide the res.statusMessage as well?
            }

What do you think @hypesystem?

[hypesystem]

Okay that's very odd behavior using the status message as a return value ...

I think we should definitely handle it. Maybe a better way of handling it would be checking that there is a JSON response object, though? That would solve the issue more generally and catch any future regressions they may make.

[eladnava]

@hypesystem Yes that could work but we do want to present the actual error message to the developer and that is only available within res.statusMessage.

How about this?

            if (!resBodyJSON) {
                debug('Invalid response received (' + res.statusCode + ' ' + res.statusMessage + ')');
                return callback(res.statusCode); // maybe we can provide the res.statusMessage as well?
            }

[hypesystem]

I think providing res.statusMessage as well is a good idea :-) potentially as an object:

return callback({ statusCode: res.statusCode, statusMessage: res.statusMessage });

I don't know how this fits with the current error messages though: they only return the statusCode right? So maybe a string is better?

return callback(res.statusCode + " " + res.statusMessage);

I don't know. Whichever you prefer :-)

[eladnava]

@hypesystem Yeah it's currently just the status code. Wouldn't this be a breaking change though?

[hypesystem]

My two cents: we have not specified what kinds of errors we send back, so we can do whatever feels "best". No matter what we are introducing a new case that people can match on to properly handle the error.

[eladnava]

OK cool, I would definitely go for the error object, seems more future-proof. Will submit PR soon.

[evolvingfunctions]

Thanks for checking in changes to address the issue! Looking forward to testing 0.14.5.

[eladnava]

Released 0.14.5. 👍

[hypesystem]

So boss! Thanks :-)