parse-community / parse-server-push-adapter

A push notification adapter for Parse Server

https://parseplatform.org

MIT License

85 stars 100 forks source link

Properly Handle Certificate Expired Error #118

Closed dplewis closed 5 years ago

dplewis commented 5 years ago

I run into this issue about twice a year where my server crashes with 500 error. It's stuck this way until a new cert is renewed.

If there is a Certificate Expired error on startup skip that push configuration.

There are more errors that could occur from the node-apn module but they rarely occur.

Let me know if there is a better way to do this.

codecov[bot] commented 5 years ago

Codecov Report

Merging #118 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #118   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         258    263    +5     
=====================================
+ Hits          258    263    +5

Impacted Files	Coverage Δ
src/APNS.js	`100% <100%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e7f9bc4...583fe6b. Read the comment docs.

flovilmart commented 5 years ago

I’m pretty much against this change as it will silently fail after and users won’t know that their certificate is expired. The best technique is to keep a sticky note as for when to rotate your certificates.

If you have SSL on, you’ll have the same issue, if you forget when to rotate them, no one will be able to access your website/API.

So for me, crashing is the right thing.

dplewis commented 5 years ago

@flovilmart I agree, Is there some way we can add an environment variable to make it optional?

flovilmart commented 5 years ago

It’s a lot of trouble for not a lot I believe. I mean, if there was a way to check the expiration and warn big time that the certificate was about to expire, that would be nice :) better than silently ignoring the cert invalidity.

The exception makes sense for me, it’s not recoverable, and the developer need to act rapidely.

dplewis commented 5 years ago

@flovilmart That would be great since I don't get emails from Apple for these kind of things. I think we can get the validity of the certs. I'll dig around some more.

dplewis commented 5 years ago

@flovilmart I opened a PR. https://github.com/node-apn/node-apn/pull/629 Let's see where this goes...

dplewis commented 5 years ago

Closing as properly handled